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

NIfTI: check consistency of sform and qform #1212

Merged
merged 7 commits into from Apr 14, 2018

Conversation

Projects
None yet
4 participants
@jdtournier
Copy link
Member

jdtournier commented Dec 7, 2017

As discussed in #1210

Could still do with added FAQ or similar in documentation...

@jdtournier jdtournier self-assigned this Dec 7, 2017

@thijsdhollander thijsdhollander self-requested a review Jan 8, 2018

@thijsdhollander
Copy link
Member

thijsdhollander left a comment

Seems all good to me! 👍

@Lestropie Lestropie added this to the Milestone 3.0_RC3 milestone Jan 17, 2018

@Lestropie Lestropie referenced this pull request Jan 17, 2018

Merged

Tag 3.0_RC3 #1232

jdtournier and others added some commits Jan 31, 2018

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 11, 2018

@jdtournier: Check that you're happy with that description in the docs.

@jdtournier

This comment has been minimized.

Copy link
Member Author

jdtournier commented Apr 12, 2018

OK, reading this FAQ entry more closely (hadn't seen it before - good find), it's clear our interpretation of these two fields (and consequently our handling of them) is not consistent with that. I'm not sure that necessarily invalidates our handling, since what the standard says (or at least Mark Jenkinson's clarification of it in that FAQ) doesn't necessarily mean that this is how other packages will behave.

Given that piece of information, this would suggest that we should use the qform preferentially if present, not the sform, since the latter is supposed to be a mapping to some standard space - not the mapping to real space, which I would argue is what we should always be using. That would be a trivial change to implement, and we could still warn the user about any discrepancies between the sform and qform, as per this PR, so users shouldn't suddenly find differences being introduced with no warning.

Note that this will only be an issue when reading NIfTI images produced by 3rd party packages that have set these two fields differently. I'm not sure whether our handling needs to change, I would have thought we'd have heard reports by now if users were finding their images poorly aligned due to MRtrix3 using the sform only and overwriting the qform in its output...?

On top of that, I note that this only states that the check should be for handedness of the transformation, not whether they actually represent the same transformation. So we are being much stricter here in our warnings than necessary, and presumably also than FSL in this respect (although they consider that a fatal error, whereas we only issue a warning, so I'd argue we're still more lenient, but you know what I mean...).

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 13, 2018

I'm happy to go with that. Users will still get a warning in the case of any ambiguity once this is merged. Docs would need to be updated in addition to the code.

@jdtournier

This comment has been minimized.

Copy link
Member Author

jdtournier commented Apr 13, 2018

I have to say, I'm not that sure. I distinctly remember someone having issues due to us using theqform preferentially (I think there was a time when that was the default behaviour), when interacting with some other package... I think maybe we should add a config file option to allow users to decide for themselves? That way we could adopt what seems to be the spirit of the standard, but allow users to maintain the current behaviour if it causes problems for them. What do you think?

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 13, 2018

I thought of a config file entry on the way home but forgot to post it... Definitely let's do that.

I agree using qform is more consistent between the standard and MRtrix3's own convention. So if a config file entry would catch that potential problematic case that was a long time ago and the details can't be remembered... I reckon go for that.

@jdtournier

This comment has been minimized.

Copy link
Member Author

jdtournier commented Apr 13, 2018

Good to hear, let's do that.

About that issue I mentioned just earlier, thinking about it some more, I think the problem was that at the time, we were only writing out the sform, and that other package relied exclusively on the qform. So it's a moot point anyway...

Lestropie added some commits Apr 14, 2018

NIfTI: Update qform/sform behaviour
- In cases where the qform and sform transformations differ in an input NIfTI image, MRtrix3 will now use the qform matrix rather than the sform.
- A new config file option 'NIfTIUseSform' gives users control over this behaviour if necessary.
The relevant section of the new documentation FAQ page entry has been updated to reflect this.
@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 14, 2018

# command: tckgen SIFT_phantom/dwi.mif -algo seedtest -seed_random_per_voxel SIFT_phantom/mask.mif 27 tmp.tck -force && tckmap tmp.tck -template SIFT_phantom/dwi.mif - | testing_diff_image - tckgen/SIFT_phantom_seeds.mif [ ERROR ]
tckgen: [WARNING] existing output files will be overwritten
tckgen: preloading data for "SIFT_phantom/dwi.mif"...  [==================================================]
tckgen: [  0%]        1 seeds,        1 streamlines,        1 selected
tckgen: [  1%]       38 seeds,       38 streamlines,       38 selected
tckgen: [  1%]       39 seeds,       39 streamlines,       39 selected
tckgen: [  2%]       79 seeds,       79 streamlines,       79 selected
tckgen: [  4%]      157 seeds,      157 streamlines,      157 selected
tckgen: [  8%]      313 seeds,      313 streamlines,      313 selected
tckgen: [ 16%]      624 seeds,      624 streamlines,      624 selected
tckgen: [ 32%]     1246 seeds,     1246 streamlines,     1246 selected
tckgen: [ 64%]     2490 seeds,     2490 streamlines,     2490 selected
tckgen: [100%]     3888 seeds,     3888 streamlines,     3888 selected
tckmap: mapping tracks to image...  [==================================================]
testing_diff_image: [ERROR] images "/tmp/mrtrix-tmp-iQigO4.mif" and "tckgen/SIFT_phantom_seeds.mif" do not match within absolute precision of 0 (28 vs 27)
testing_diff_image: [ERROR] images "/tmp/mrtrix-tmp-iQigO4.mif" and "tckgen/SIFT_phantom_seeds.mif" do not match within absolute precision of 0 (26 vs 27)
testing_diff_image: [ERROR] exception thrown from one or more threads "loop threads"

o.0

Edit: Tests passed by restarting. There's a race condition residing in there somewhere... I'm sure it's not the first time I've seen this.

@Lestropie Lestropie merged commit bac085e into dev Apr 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Lestropie Lestropie deleted the check_nifti_qform_sform_consistency branch Apr 14, 2018

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Jan 25, 2019

Just discovered another confound related to this and other issues.

SPM's "Coregister: Estimate" functionality, which is commonly used for inter-modality intra-subject rigid-body registration, directly modifies the sform of the input NIfTI, but does not modify the qform. As such, if MRtrix3 loads the qform, while a command-line warning is issued about this fact, such warnings are often ignored, and the image will appear in mrview as though the registration has not yet been performed.

@maxpietsch

This comment has been minimized.

Copy link
Member

maxpietsch commented Jan 25, 2019

ANTs' (and ITK's) NIfTI handling differs again:
Therefore it can only use qform when reading a NIFTI-1 image. When writing NIFTI output, only the qform is written; the sform is replaced with zeros.
https://github.com/ANTsX/ANTs/wiki/How-does-ANTs-handle-qform-and-sform-in-NIFTI-1-images%3F

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Jan 29, 2019

When writing NIFTI output, only the qform is written; the sform is replaced with zeros.

That's not so much of a concern; if anything MRtrix3 is quite happy if one is set and the other is not. The problem with SPM is specifically because it modifies the sform in place but leaves the qform untouched. So if the qform is filled with valid data before that operation takes place, then after that operation, both will be flagged as being valid and both will contain valid data, yet they will be different from one another; hence the ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment