NF: add SH basis conversion between dipy and mrtrix3 #3008

ebrahimebrahim · 2023-12-12T16:16:27Z

This adds a function convert_sh_descoteaux_tournier to convert SH representations between DIPY and MRtrix3.

(There was also a plan to add something to add an associated workflow to dipy.workflows.io, but I don't feel I understand the workflow concept well enough to add that. Not doing it now.)

pep8speaks · 2023-12-12T16:16:34Z

Hello @ebrahimebrahim, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2024-01-11 19:18:39 UTC

ebrahimebrahim · 2023-12-12T16:19:46Z

Hello @ebrahimebrahim, Thank you for submitting the Pull Request !

In the file dipy/reconst/shm.py:

Line 1615:81: E501 line too long (94 > 80 characters)
Line 1616:81: E501 line too long (96 > 80 characters)

Do see the DIPY coding Style guideline

(these are URLs so it will have to violate pep8)

skoudoro · 2023-12-12T16:29:27Z

Hi @ebrahimebrahim,

Thank you doing this!

concerning the violating pep8, just add # noqa: E501 at the end of the violating line.

concerning the workflow, you need

to add a class that will call your function. look at this tutorial (the part with the bin folder is deprecated so ignore)
define the command line name in pyproject.toml. look here
add your workflow name and cli name in dipy.workflows.cli. look here

ebrahimebrahim · 2023-12-12T21:01:14Z

I tried out the CLI workflow on an example case. It seems to be all set. Thanks for explaining workflows!

dipy/workflows/io.py

dipy/workflows/cli.py

jhlegarreta · 2023-12-12T21:44:51Z

Thanks for doing this @ebrahimebrahim.

codecov · 2023-12-12T22:47:58Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (191d34c) 82.11% compared to head (64cdb0d) 82.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3008      +/-   ##
==========================================
+ Coverage   82.11%   82.12%   +0.01%     
==========================================
  Files         146      146              
  Lines       20495    20513      +18     
  Branches     3268     3270       +2     
==========================================
+ Hits        16829    16846      +17     
- Misses       2855     2856       +1     
  Partials      811      811

Files	Coverage Δ
dipy/reconst/shm.py	`93.62% <100.00%> (+0.12%)`	⬆️
dipy/workflows/cli.py	`42.85% <ø> (ø)`
dipy/workflows/io.py	`76.33% <90.90%> (+1.33%)`	⬆️

ebrahimebrahim · 2023-12-13T06:26:31Z

Hmm the code coverage thing is not happy with me because of "patch" percent coverage. Maybe it does not count mock objects?

skoudoro

Hi @ebrahimebrahim,

No worries for codecov. you are ok following this: https://app.codecov.io/gh/dipy/dipy/pull/3008/commits

see below my comment.

dipy/workflows/tests/test_io.py

skoudoro

Thank you for the update @ebrahimebrahim.

See below for my comment.

is there any additional comment concerning this PR? @arokem?

dipy/workflows/tests/test_io.py

ebrahimebrahim · 2023-12-19T19:24:43Z

Any idea why the windows tests are failing after using os.path.join?

FAILED workflows/tests/test_io.py::test_convert_sh_flow - NotADirectoryError: [WinError 267] The directory name is invalid: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmp4f8uynzy\\sh_coeff_img_converted.nii'

skoudoro · 2023-12-19T19:29:07Z

Any idea why the windows tests are failing after using os.path.join?

Interesting.... no idea, I need to connect to Windows machine and see what's going on. Unfortunately, I can do it only next week...

ebrahimebrahim · 2023-12-19T19:33:34Z

Any idea why the windows tests are failing after using os.path.join?

Interesting.... no idea, I need to connect to Windows machine and see what's going on. Unfortunately, I can do it only next week...

Thanks, and no rush at all! I appreciate the testing effort as I don't have easy access to a windows workstation for a while.

There may be something I've misunderstood about workflows and out_dir. From the error message it looks like the path C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmp4f8uynzy\\sh_coeff_img_converted.nii was expected to be a directory for some reason.

skoudoro

Hi @ebrahimebrahim,

Happy New Year ! sorry for the late feedback see below my comment. this should fix your issue for now.

dipy/workflows/tests/test_io.py

skoudoro · 2024-01-05T16:01:48Z

Also, Can you update this line in the documentation: https://github.com/dipy/dipy/blob/master/doc/recipes.rst?plain=1#L35-L36

Thank you

ebrahimebrahim · 2024-01-08T20:58:51Z

This should be ready for another look. It is rebased to master.

If I understand the output correctly from the two failing checks, I think in both cases it's not something related to this PR.

skoudoro

Hi @ebrahimebrahim,

Thank you again for this. it looks good to me.

I will wait until Friday to see if we have a feedback from @jhlegarreta or @arokem or someone else. If not, I will go ahead and merge it.

Thank you for this work.

ps: during this time, if you could fix the small docstring issue, it would be great. Thanks!

dipy/reconst/shm.py

jhlegarreta

I have a style comment. Not a show-stopper, but would be nice to address as Serge has also requested a change.

Thanks for doing this @ebrahimebrahim.

dipy/reconst/shm.py

jhlegarreta

IMO, all commits in this PR should be squashed and all meaningful changes be explained in that single commit as this is a new feature: e.g. without commits 035858f and 58d6cba we would be introducing commits that do not work/do not pass tests in the history.

Would reduce the number of commits in the history, and thus make it less complex and save time when reading through it.

skoudoro · 2024-01-11T14:45:50Z

Concerning the squash, I agree with @jhlegarreta, but it is a bonus, I think it is fine if you can not do it.

ebrahimebrahim · 2024-01-11T18:03:39Z

Comments addressed and commits squashed

skoudoro · 2024-01-11T18:16:19Z

Great, thanks! waiting for the CI's, then we can merge

jhlegarreta

I am not going to oppose to merging this, but the commit message is not faithful to the change or NF: it has simply used the previous commit message subjects. Thus, e.g. saying that .gz was added to the .nii extensions, that long line or URL PEP8 is bypassed, that the function naming is improved or that os.path was used instead of pathlib is now meaningless. This is now a single commit, and readers are interested in the final result or added feature; the intermediate steps or process does not add meaningful information to the commit history; it is only relevant to the GitHub conversation.

So a self-contained commit message could be along the following lines:

NF: Add DIPY to MRtrix3 SH basis conversion capabilities

Add legacy descoteaux07 (DIPY) and tournier07 (MRtrix3) SH basis conversion capabilities.

Add the corresponding tests.

Add and test the corresponding workflow.

Document the feature in the DIPY how-to recipes file.

Hope this makes sense.

Add legacy descoteaux07 (DIPY) and tournier07 (MRtrix3) SH basis conversion capabilities. Add the corresponding tests. Add and test the corresponding workflow. Document the feature in the DIPY how-to recipes file.

ebrahimebrahim · 2024-01-11T19:18:35Z

So a self-contained commit message could be along the following lines:

Thanks, I agree with your reasoning and like your rephrasing. I copied your message :)

jhlegarreta

Thanks for doing this and for addressing the comments @ebrahimebrahim 💯.

jhlegarreta · 2024-01-11T22:27:03Z

macOS VIZ failure is unrelated. Merging.

skoudoro added the type:New feature label Dec 12, 2023

jhlegarreta reviewed Dec 12, 2023

View reviewed changes

dipy/workflows/io.py Outdated Show resolved Hide resolved

dipy/workflows/cli.py Outdated Show resolved Hide resolved

skoudoro requested changes Dec 13, 2023

View reviewed changes

dipy/workflows/tests/test_io.py Outdated Show resolved Hide resolved

ebrahimebrahim requested a review from skoudoro December 13, 2023 20:41

skoudoro reviewed Dec 13, 2023

View reviewed changes

dipy/workflows/tests/test_io.py Outdated Show resolved Hide resolved

dipy/workflows/tests/test_io.py Outdated Show resolved Hide resolved

ebrahimebrahim requested a review from skoudoro December 13, 2023 22:04

skoudoro requested changes Jan 5, 2024

View reviewed changes

dipy/workflows/tests/test_io.py Outdated Show resolved Hide resolved

ebrahimebrahim force-pushed the add-dipy-mrtrix-shm-conversion branch from 0c44261 to a2fbd4b Compare January 8, 2024 17:55

ebrahimebrahim requested a review from skoudoro January 8, 2024 20:59

skoudoro approved these changes Jan 10, 2024

View reviewed changes

dipy/reconst/shm.py Outdated Show resolved Hide resolved

jhlegarreta reviewed Jan 10, 2024

View reviewed changes

dipy/reconst/shm.py Show resolved Hide resolved

jhlegarreta reviewed Jan 11, 2024

View reviewed changes

ebrahimebrahim force-pushed the add-dipy-mrtrix-shm-conversion branch from 616a781 to f610ced Compare January 11, 2024 18:02

jhlegarreta reviewed Jan 11, 2024

View reviewed changes

NF: Add DIPY to MRtrix3 SH basis conversion capabilities

64cdb0d

Add legacy descoteaux07 (DIPY) and tournier07 (MRtrix3) SH basis conversion capabilities. Add the corresponding tests. Add and test the corresponding workflow. Document the feature in the DIPY how-to recipes file.

ebrahimebrahim force-pushed the add-dipy-mrtrix-shm-conversion branch from f610ced to 64cdb0d Compare January 11, 2024 19:18

jhlegarreta approved these changes Jan 11, 2024

View reviewed changes

jhlegarreta merged commit 443f702 into dipy:master Jan 11, 2024
29 of 30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NF: add SH basis conversion between dipy and mrtrix3 #3008

NF: add SH basis conversion between dipy and mrtrix3 #3008

ebrahimebrahim commented Dec 12, 2023 •

edited

pep8speaks commented Dec 12, 2023 •

edited

ebrahimebrahim commented Dec 12, 2023

skoudoro commented Dec 12, 2023 •

edited

ebrahimebrahim commented Dec 12, 2023

jhlegarreta commented Dec 12, 2023

codecov bot commented Dec 12, 2023 •

edited

ebrahimebrahim commented Dec 13, 2023

skoudoro left a comment

skoudoro left a comment

ebrahimebrahim commented Dec 19, 2023

skoudoro commented Dec 19, 2023

ebrahimebrahim commented Dec 19, 2023

skoudoro left a comment

skoudoro commented Jan 5, 2024

ebrahimebrahim commented Jan 8, 2024 •

edited

skoudoro left a comment •

edited

jhlegarreta left a comment

jhlegarreta left a comment

skoudoro commented Jan 11, 2024

ebrahimebrahim commented Jan 11, 2024

skoudoro commented Jan 11, 2024

jhlegarreta left a comment

ebrahimebrahim commented Jan 11, 2024

jhlegarreta left a comment

jhlegarreta commented Jan 11, 2024

NF: add SH basis conversion between dipy and mrtrix3 #3008

NF: add SH basis conversion between dipy and mrtrix3 #3008

Conversation

ebrahimebrahim commented Dec 12, 2023 • edited

pep8speaks commented Dec 12, 2023 • edited

Comment last updated at 2024-01-11 19:18:39 UTC

ebrahimebrahim commented Dec 12, 2023

skoudoro commented Dec 12, 2023 • edited

ebrahimebrahim commented Dec 12, 2023

jhlegarreta commented Dec 12, 2023

codecov bot commented Dec 12, 2023 • edited

Codecov Report

ebrahimebrahim commented Dec 13, 2023

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro left a comment

Choose a reason for hiding this comment

ebrahimebrahim commented Dec 19, 2023

skoudoro commented Dec 19, 2023

ebrahimebrahim commented Dec 19, 2023

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro commented Jan 5, 2024

ebrahimebrahim commented Jan 8, 2024 • edited

skoudoro left a comment • edited

Choose a reason for hiding this comment

jhlegarreta left a comment

Choose a reason for hiding this comment

jhlegarreta left a comment

Choose a reason for hiding this comment

skoudoro commented Jan 11, 2024

ebrahimebrahim commented Jan 11, 2024

skoudoro commented Jan 11, 2024

jhlegarreta left a comment

Choose a reason for hiding this comment

ebrahimebrahim commented Jan 11, 2024

jhlegarreta left a comment

Choose a reason for hiding this comment

jhlegarreta commented Jan 11, 2024

ebrahimebrahim commented Dec 12, 2023 •

edited

pep8speaks commented Dec 12, 2023 •

edited

skoudoro commented Dec 12, 2023 •

edited

codecov bot commented Dec 12, 2023 •

edited

ebrahimebrahim commented Jan 8, 2024 •

edited

skoudoro left a comment •

edited