Use the correct (row) order of the tensor components #1892

arokem · 2019-07-09T23:02:49Z

No description provided.

arokem · 2019-07-09T23:07:14Z

Addresses #1890

codecov-io · 2019-07-10T04:48:51Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a3649ef). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1892   +/-   ##
=========================================
  Coverage          ?   84.68%           
=========================================
  Files             ?      118           
  Lines             ?    14682           
  Branches          ?     2327           
=========================================
  Hits              ?    12433           
  Misses            ?     1703           
  Partials          ?      546

Impacted Files	Coverage Δ
dipy/workflows/reconst.py	`77.45% <100%> (ø)`

ecaruyer · 2019-07-10T05:29:16Z

Thanks @arokem for this PR; if we wanted to be fully compliant with the Nifti1 standard, there would still be 2 things to modify:

providing the right intent code in the header (using something like hdr.set_intent("symmetric matrix") in nibabel)
adding one more dimension to the image data (using something like data = data[..., np.newaxis, :]) -- as far as I understand the 4th dimension is dedicated to store temporal data, e.g. for fMRI.

This would be cherry on the cake, but as it stands this PR solves #1890

arokem · 2019-07-10T13:45:47Z

Thanks for the feedback. I’ll do those things later today. I also want to add a little test to be sure we get this right.

…

On Tue, Jul 9, 2019 at 10:29 PM Emmanuel Caruyer ***@***.***> wrote: Thanks @arokem <https://github.com/arokem> for this PR; if we wanted to be fully compliant with the Nifti1 standard, there would still be 2 things to modify: - providing the right intent code in the header (using something like hdr.set_intent("symmetric matrix") in nibabel) - adding one more dimension to the image data (using something like data = data[..., np.newaxis, :]) -- as far as I understand the 4th dimension is dedicated to store temporal data, e.g. for fMRI. This would be cherry on the cake, but as it stands this PR solves #1890 <#1890> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1892>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA46NU2Z3AQ54JCHB3DAVTP6VXS3ANCNFSM4H7J7KLQ> .

jchoude · 2019-07-10T18:30:04Z

Just a quick mention: I fully agree that we should follow the Nifti standard (using the 5 dimension for the elements, since the 4th is for temporal), and I even often argued that this should be the case. However, I was most often told that "other software expect the values to be in the 4th dimension". Same goes for the SH coefficients, etc.

If we change it to the 5th dimension, I expect to see a few issues coming about external software not loading that correctly anymore. Just something to take into account.

Also, @matthieudumont is probably not seeing those, even if he's sitting at the desk besides mine. I'm pretty sure that the reason the reordering was done was to conform to what the Fibernavigator expected at the time. I personally would say that this is not a valid reason anymore. Just legacy.

jhlegarreta · 2019-07-11T15:21:56Z

dipy/workflows/reconst.py

@@ -280,6 +287,8 @@ def run(self, input_files, bvalues_files, bvectors_files, mask_files,
            (default 'evecs.nii.gz')
        out_eval : string, optional
            Name of the eigenvalues to be saved (default 'evals.nii.gz')
+        fsl_format : bool, optional
+            Whether the tensor


The parameter explanation here seems to be unfinished 📖.

arokem · 2019-07-11T15:25:31Z

Good catch! See last commit.

…

On Thu, Jul 11, 2019 at 8:21 AM Jon Haitz Legarreta Gorroño < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dipy/workflows/reconst.py <#1892 (comment)>: > @@ -280,6 +287,8 @@ def run(self, input_files, bvalues_files, bvectors_files, mask_files, (default 'evecs.nii.gz') out_eval : string, optional Name of the eigenvalues to be saved (default 'evals.nii.gz') + fsl_format : bool, optional + Whether the tensor The parameter explanation here seems to be unfinished 📖. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1892>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA46NQSMYW5JSXMBPAT7BDP65FZLANCNFSM4H7J7KLQ> .

jhlegarreta · 2019-07-11T15:29:47Z

Nice. Thanks @arokem.

pep8speaks · 2019-07-13T03:38:33Z

Hello @arokem, Thank you for updating !

In the file dipy/workflows/reconst.py:

Line 226:81: E501 line too long (81 > 80 characters)

Comment last updated at 2019-07-29 20:49:57 UTC

arokem · 2019-07-15T15:41:26Z

Hey @jchoude : does this allay your concerns? The default might break other software, but there's a flag.

@MrBago : I understand the concern about back compatibility, but with 1.0 as our coming release, I think this is the time to break back compatibility if there ever was one. I am not sure what issues would come up, but we'd help the Nipype-ers refactor as needed.

MrBago · 2019-07-15T20:03:39Z

Sounds good.

…

On Mon, Jul 15, 2019 at 8:41 AM Ariel Rokem ***@***.***> wrote: Hey @jchoude <https://github.com/jchoude> : does this allay your concerns? The default might break other software, but there's a flag. @MrBago <https://github.com/MrBago> : I understand the concern about back compatibility, but with 1.0 as our coming release, I think this is the time to break back compatibility if there ever was one. I am not sure what issues would come up, but we'd help the Nipype-ers refactor as needed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1892>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABWP4ZKOOXOTSY6AT655ZDP7SLCXANCNFSM4H7J7KLQ> .

skoudoro · 2019-07-17T17:46:19Z

Can you rebase this PR? Thank you!

Use the nifti standard compliant form as default.

arokem · 2019-07-25T13:36:19Z

Rebased!

skoudoro · 2019-07-26T14:45:40Z

Thank you @arokem. Any other comments? I will wait until tomorrow before merging this PR.

Garyfallidis · 2019-07-26T18:11:34Z

dipy/workflows/reconst.py

@@ -222,7 +223,7 @@ def run(self, input_files, bvalues_files, bvectors_files, mask_files,
            out_dir='', out_tensor='tensors.nii.gz', out_fa='fa.nii.gz',
            out_ga='ga.nii.gz', out_rgb='rgb.nii.gz', out_md='md.nii.gz',
            out_ad='ad.nii.gz', out_rd='rd.nii.gz', out_mode='mode.nii.gz',
-            out_evec='evecs.nii.gz', out_eval='evals.nii.gz'):
+            out_evec='evecs.nii.gz', out_eval='evals.nii.gz', fsl_tensor=False):


The name of the parameter is misleading. Should we say nifti_standard_tensor or nifti_tensor?

And this would be set to True per default? The problem with that is that it's not so clear what the alternative means. Here, the idea is that this tells the users that this would give them a file that interoperates with FSL. Does that make sense?

Trying to use software names in the past did not go well. Why fsl_tensor and not trackvis_tensor or other? I think having the nifti_standard there is better and you can explain more in the docstring. And in the docstring we should have the convention explained for the choice of lower vs upper triangular and row vs column order. @ecaruyer do you agree?

How's this? See recent 24dc0ea

Yes, I agree with the nifti_standard suggestion.

Just one last comment from me, I am quite sure trackvis does follow the row-order lower triangular convention, which is I think in contradiction with the docstring as it stands.

Thanks again for this PR!

Oh yeah. I think the other software that was mentioned was Fibernavigator, but I think even that may no longer be true. Fixed in 162de19

Garyfallidis · 2019-07-30T20:43:26Z

Great, thank you all. Merging...

arokem mentioned this pull request Jul 9, 2019

Tensor I/O in dipy_fit_dti #1890

Closed

jhlegarreta reviewed Jul 11, 2019

View reviewed changes

skoudoro added this to the 1.0 milestone Jul 11, 2019

arokem force-pushed the fix-1890 branch from 6efa1a6 to 317cd7d Compare July 13, 2019 03:38

arokem added 5 commits July 25, 2019 06:35

Use the correct (row) order of the tensor components

5ddafb5

Enable "FSL format" as an option.

344e876

Use the nifti standard compliant form as default.

Complete the docstring.

60c2c6c

Adds testing for FSL tensor format.

240b8b0

PEP8

33a9a75

arokem force-pushed the fix-1890 branch from b67a11e to 33a9a75 Compare July 25, 2019 13:36

Garyfallidis reviewed Jul 26, 2019

View reviewed changes

arokem added 3 commits July 29, 2019 13:31

Call the kwarg nifti_tensor and set to default True.

24dc0ea

This fixes the test.

0b4809d

Remove Trackviz from this reference.

162de19

Garyfallidis merged commit c2e5121 into master Jul 30, 2019

skoudoro deleted the fix-1890 branch August 7, 2019 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the correct (row) order of the tensor components #1892

Use the correct (row) order of the tensor components #1892

arokem commented Jul 9, 2019

arokem commented Jul 9, 2019

codecov-io commented Jul 10, 2019 •

edited

Loading

ecaruyer commented Jul 10, 2019

arokem commented Jul 10, 2019 via email

jchoude commented Jul 10, 2019

jhlegarreta Jul 11, 2019

arokem commented Jul 11, 2019 via email

jhlegarreta commented Jul 11, 2019

pep8speaks commented Jul 13, 2019 •

edited

Loading

arokem commented Jul 15, 2019

MrBago commented Jul 15, 2019 via email

skoudoro commented Jul 17, 2019

arokem commented Jul 25, 2019

skoudoro commented Jul 26, 2019

Garyfallidis Jul 26, 2019

arokem Jul 28, 2019

Garyfallidis Jul 29, 2019

arokem Jul 29, 2019

ecaruyer Jul 29, 2019 •

edited

Loading

arokem Jul 29, 2019

Garyfallidis commented Jul 30, 2019

Use the correct (row) order of the tensor components #1892

Use the correct (row) order of the tensor components #1892

Conversation

arokem commented Jul 9, 2019

arokem commented Jul 9, 2019

codecov-io commented Jul 10, 2019 • edited Loading

Codecov Report

ecaruyer commented Jul 10, 2019

arokem commented Jul 10, 2019 via email

jchoude commented Jul 10, 2019

jhlegarreta Jul 11, 2019

Choose a reason for hiding this comment

arokem commented Jul 11, 2019 via email

jhlegarreta commented Jul 11, 2019

pep8speaks commented Jul 13, 2019 • edited Loading

Comment last updated at 2019-07-29 20:49:57 UTC

arokem commented Jul 15, 2019

MrBago commented Jul 15, 2019 via email

skoudoro commented Jul 17, 2019

arokem commented Jul 25, 2019

skoudoro commented Jul 26, 2019

Garyfallidis Jul 26, 2019

Choose a reason for hiding this comment

arokem Jul 28, 2019

Choose a reason for hiding this comment

Garyfallidis Jul 29, 2019

Choose a reason for hiding this comment

arokem Jul 29, 2019

Choose a reason for hiding this comment

ecaruyer Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

arokem Jul 29, 2019

Choose a reason for hiding this comment

Garyfallidis commented Jul 30, 2019

codecov-io commented Jul 10, 2019 •

edited

Loading

pep8speaks commented Jul 13, 2019 •

edited

Loading

ecaruyer Jul 29, 2019 •

edited

Loading