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

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

Merged
merged 8 commits into from Jul 30, 2019

Conversation

@arokem
Copy link
Member

commented Jul 9, 2019

No description provided.

@arokem

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Addresses #1890

@arokem arokem referenced this pull request Jul 9, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Jul 10, 2019

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

@jchoude

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

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.

@@ -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

This comment has been minimized.

Copy link
@jhlegarreta

jhlegarreta Jul 11, 2019

Contributor

The parameter explanation here seems to be unfinished 📖.

@arokem

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

Nice. Thanks @arokem.

@skoudoro skoudoro added this to the 1.0 milestone Jul 11, 2019

@arokem arokem force-pushed the fix-1890 branch from 6efa1a6 to 317cd7d Jul 13, 2019

@pep8speaks

This comment has been minimized.

Copy link

commented Jul 13, 2019

Hello @arokem, Thank you for updating !

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

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

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Can you rebase this PR? Thank you!

@arokem arokem force-pushed the fix-1890 branch from b67a11e to 33a9a75 Jul 25, 2019

@arokem

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

Rebased!

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

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

@@ -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):

This comment has been minimized.

Copy link
@Garyfallidis

Garyfallidis Jul 26, 2019

Member

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

This comment has been minimized.

Copy link
@arokem

arokem Jul 28, 2019

Author Member

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?

This comment has been minimized.

Copy link
@Garyfallidis

Garyfallidis Jul 29, 2019

Member

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?

This comment has been minimized.

Copy link
@arokem

arokem Jul 29, 2019

Author Member

How's this? See recent 24dc0ea

This comment has been minimized.

Copy link
@ecaruyer

ecaruyer Jul 29, 2019

Contributor

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!

This comment has been minimized.

Copy link
@arokem

arokem Jul 29, 2019

Author Member

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

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Great, thank you all. Merging...

@Garyfallidis Garyfallidis merged commit c2e5121 into master Jul 30, 2019

6 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the fix-1890 branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.