Skip to content

Conversation

@oesteban
Copy link
Member

After much thought, I have come to the conclusion that the VOX2RAS affine must be applied in all cases, regardless of whether the dataset is oblique or not.

After all, the VSM (voxel-shift-map) is no more than a regularly gridded field of voxel coordinates. The transformation between voxels and mm is biyective and univocal, and formalized by the affine matrix of the image.

This PR simplifies the calculation, and theoretically should resolve most of the issues we are experiencing when resampling data.

Resolves: #218.
Resolves: #236.

Related: nipreps/fmriprep#2210.

@oesteban oesteban added the bug Something isn't working label Sep 30, 2021
@oesteban oesteban added this to the 2.0.7 milestone Sep 30, 2021
@oesteban oesteban requested review from effigies and mgxd September 30, 2021 08:55
@oesteban
Copy link
Member Author

@mgxd this (in combination with the latest fMRIPrep integration) should fix things for your dataset - please let me know

@oesteban oesteban force-pushed the fix/field-generation branch from 2b45237 to fd74249 Compare September 30, 2021 09:43
@oesteban oesteban marked this pull request as draft September 30, 2021 09:44
@oesteban
Copy link
Member Author

I'm writing some unit tests, just to be sure this is what we need.

After much thought, I have come to the conclusion that the VOX2RAS
affine must be applied in all cases, regardless of whether the dataset
is oblique or not.

After all, the VSM (voxel-shift-map) is no more than a regularly gridded
field of voxel coordinates.
The transformation between voxels and mm is biyective and univocal, and
formalized by the affine matrix of the image.

This PR simplifies the calculation, and theoretically should resolve
most of the issues we are experiencing when resampling data.

Resolves: #218.
Resolves: #236.

Related: nipreps/fmriprep#2210.
@oesteban oesteban force-pushed the fix/field-generation branch from fd74249 to 5a8a75c Compare September 30, 2021 12:28
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Made some very small suggestions, but feel free to reject without argument.

Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>
@oesteban
Copy link
Member Author

I'm writing a test with a numerical phantom. I have been able to compare the correction with our B0FieldTransform object (i.e., in voxel space) and with antsApplyTransforms using the displacements field our object generates. With RAS, canonical affine it works. Will configure the test to explore obliquity, flips and axis ordering.

Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>
@oesteban oesteban force-pushed the fix/field-generation branch from e38f01b to 3642514 Compare September 30, 2021 16:17
@oesteban oesteban force-pushed the fix/field-generation branch from f36338e to d9c142b Compare September 30, 2021 21:47
@oesteban oesteban marked this pull request as ready for review September 30, 2021 21:47
@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2021

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.28%. Comparing base (6dbf688) to head (d9c142b).

Files with missing lines Patch % Lines
sdcflows/transform.py 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage   85.31%   85.28%   -0.04%     
==========================================
  Files          25       25              
  Lines        1771     1767       -4     
  Branches      205      205              
==========================================
- Hits         1511     1507       -4     
  Misses        230      230              
  Partials       30       30              
Flag Coverage Δ
travis 85.28% <93.75%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving again. Very convincing test!

@oesteban
Copy link
Member Author

oesteban commented Oct 1, 2021

Very convincing test!

Yup, it doesn't tell you that we have a good implementation of fieldmaps, but we are safe to say that, once you have the fieldmap our tool and ANTs will apply the transform the same. So it indeed resolves the unit test :)

Merging in, we can include @mgxd on his data later down the line.

@oesteban oesteban merged commit 7d5b6d7 into master Oct 1, 2021
@oesteban oesteban deleted the fix/field-generation branch October 1, 2021 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

4 participants