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

Tensor I/O in dipy_fit_dti #1890

Closed
ecaruyer opened this issue Jul 8, 2019 · 13 comments

Comments

@ecaruyer
Copy link
Contributor

commented Jul 8, 2019

Short description

When exporting a tensor field using dipy_fit_dti, the order in not the standard row-order lower-triangular. There is a NIFTI standard for storing symmetric matrices (see below), I would suggest to stick to the standard, although there is a good reason not to do so. Another argument is that in dipy.reconst.dti, there is a from_lower_triangular() method which is expecting row-order.

In the code

In the source code of ReconstDtiFlow, a correct_order = [0, 1, 3, 2, 4, 5] is introduced, which basically changes the row-order lower triangular into a column-order lower triangular. Could you guide me to the rational behind this reordering?

Reference to NIFTI standard

 /*! To store an NxN symmetric matrix at each voxel:
       - dataset must have a 5th dimension
       - intent_code must be NIFTI_INTENT_SYMMATRIX
       - dim[5] must be N*(N+1)/2
       - intent_p1 must be N (in float format)
       - the matrix values A[i][[j] are stored in row-order:
         - A[0][0]
         - A[1][0] A[1][1]
         - A[2][0] A[2][1] A[2][2]
         - etc.: row-by-row                           */

#define NIFTI_INTENT_SYMMATRIX 1005

(source Official definition of the nifti1 header)

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@arokem and @MrBago please respond to @ecaruyer and we can look if it is possible to export to the current or nifti standard way. Adding a function that does the reordering should be easy right?

@arokem

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Maybe @matthieudumont knows? He wrote that line: 63dc1be

Does he still hang out here? I'd be +1 to changing this to the "correct" order.

@arokem

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

See #1892

@arokem

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Hey @ecaruyer : just to make sure. You wrote "... I would suggest to stick to the standard, although there is a good reason not to do so...". Did you mean " I would suggest to stick to the standard,unless there is a good reason not to do so"?

@ecaruyer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Hi @arokem; indeed :-). Thanks for the discussion and for your responsiveness!

@jchoude

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Regarding @matthieudumont I explained the reasoning in #1892

@MrBago

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

We have a function that'll build a properly formed nifti symmetric matrix from an array:

def nifti1_symmat(image_data, *args, **kwargs):

but this part of the nifti spec is rarely used in practice. My memory is that we picked this representation for compatibility with other software, I quickly verified that both trackvis & fsl use a 4d nifti image with the 6 tensor elements along the 4th dimension.

@arokem

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Hey @MrBago:

both trackvis & fsl use a 4d nifti image with the 6 tensor elements along the 4th dimension.

Are the elements for these at least ordered according to the nifti standard? We could use the correct ordering but in the 4th dimension as a compromise.

@MrBago

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

FSL explicitly does not use the ordering specified in the nifti spec. Lower triangular order would be Dxx, Dxy, Dyy, Dxz, Dyz, Dzz FSL uses Dxx, Dxy, Dxz, Dyy, Dyz, Dzz. Notice the 2 and 3 positions (yy, & xz) are switched. That's why we change the ordering before saving.

I believe trackvis follows the FSL ordering.

We could do what dti_recon does. They have a flag --output_5d, which users can specify if they want the tensor saved according to the nifti spec. I don't love the name of their parameter, I'd go with something more explicit like "--save-tensor-as-nifti-symmat"

@arokem

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@MrBago

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

I'm no longer an active developer or user of dipy so my opinion should only carry limited weight, but ...

I think backwards compatibility and interoperability trump the formal spec. iirc, nipype has wrapped "dipy_fit_dti" as a node and if any users are using the output of this method as input into some fsl method (either through nipype or or their own scripts) this change will break their workflows. Also, I don't know of any software that uses nifti symmat as input, I'd be reluctant to pick a default format that's not supported by anyone else.

my 2 cents.

@ecaruyer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Maybe we save the tensor in the nifti symmat format per default and add a --fsl-format flag instead

+1

I believe trackvis follows the FSL ordering.

Actually this is while using trackvis's dti_tracker with a tensor image created with dipy_fit_dti that I had this issue we are commenting on. Trackvis is indeed expecting the standard Nifti row-order lower triangular.

I think backwards compatibility and interoperability trump the formal spec.

I would say that standards are here to enable/facilitate interoperability :-).

About backward compatibility, there used to be a bin/dipy_fit_tensor which was removed in #1087 ; that script was following Nifti standard, at least for the order of the elements.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

fixed by #1892, closing

@skoudoro skoudoro closed this Jul 30, 2019

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

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