Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOC: Documentation for Eddymotion Python Module Usage #169

Merged
merged 13 commits into from
May 17, 2024

Conversation

esavary
Copy link
Member

@esavary esavary commented Apr 16, 2024

This pull request adds instructions on integrating Eddymotion functionalities with custom diffusion-weighted imaging (DWI) data within Python modules. By following these steps, users can incorporate Eddymotion functionnalities into their projects. Closes issue #126.

@esavary
Copy link
Member Author

esavary commented Apr 16, 2024

There are some linting errors spotted by Ruff in the CircleCI routine that are not related to changes in this PR but rather to modifications made previously in update_authors.py. If it's okay, I can write an issue or fix them in a separate PR.

@jhlegarreta
Copy link
Collaborator

#169 (comment) should be fixed by PR #172.

Copy link
Collaborator

@arokem arokem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small suggestion to make it clear that this is not limited to modules that get imported elsewhere, but can also be used in "scripting"

docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not have time to render it locally, so my review is partial. Left a few comments.

docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
@esavary esavary requested a review from oesteban April 22, 2024 09:32
@esavary
Copy link
Member Author

esavary commented Apr 22, 2024

I did not have time to render it locally, so my review is partial. Left a few comments.

Thanks, are you planning to conduct a more extensive review later when you have more time, or should we consider merging the PR and make improvements in a subsequent PR?

@oesteban
Copy link
Member

should we consider merging the PR and make improvements in a subsequent PR?

I don't see any benefit to this route -- IMHO, this doesn't seem that urgent to merge and rework as we go.

I cannot review it now, but I will try to find the time.

@effigies
Copy link
Member

As a non-user, I think this reads well. It would be nice if there were a link to some data (small enough to run reasonably quickly) I could download to test that all of the commands work as advertised.

@esavary
Copy link
Member Author

esavary commented Apr 25, 2024

As a non-user, I think this reads well. It would be nice if there were a link to some data (small enough to run reasonably quickly) I could download to test that all of the commands work as advertised.

Thanks, I've added a note to retrieve an example of DWI data from OSF in 4762fad. However, I'm not sure if there might be a better choice of test data, perhaps something lighter. This is one of the datasets I'm currently using locally to test the code.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a couple of small changes (I pushed directly), I was able to follow along.

@effigies effigies merged commit de1b535 into nipreps:main May 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants