Skip to content

FIX: Ordering of ITK displacements fields when reading and writing #266

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

FIX: Ordering of ITK displacements fields when reading and writing #266

wants to merge 6 commits into from
+271 −36

Conversation

oesteban
Copy link
Collaborator

Transposes data as suggested by @feilong when reading and writing ITK displacements fields.

Tests have been added to ensure round-trip consistency.

Related-to: #171.

@oesteban oesteban force-pushed the fix/itk-displacements-field-round-trip branch 2 times, most recently from e4e2b0a to 076c4fa Compare July 24, 2025 14:55
@oesteban
Copy link
Collaborator Author

@feilong apologies for the noise of the force-pushes

@codecov Codecov
Copy link

codecov bot commented Jul 24, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (60168b2) to head (6d32a6f).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
nitransforms/io/itk.py 0.00% 19 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (60168b2) and HEAD (6d32a6f). Click for more details.

HEAD has 7 uploads less than BASE
Flag BASE (60168b2) HEAD (6d32a6f)
7 1
unittests 1 0
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #266       +/-   ##
==========================================
- Coverage   95.10%   0.00%   -95.11%     
==========================================
  Files          16      16               
  Lines        1982    1991        +9     
  Branches      265       0      -265     
==========================================
- Hits         1885       0     -1885     
- Misses         76    1991     +1915     
+ Partials       21       0       -21
Flag Coverage Δ
unittests ?

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.

@feilong
Copy link
Contributor

feilong commented Jul 24, 2025

@oesteban No worries. Happy to see the issue fixed in nitransforms.

oesteban added a commit that referenced this pull request Jul 25, 2025
@oesteban
wip: disable tests that will be addressed by #266
140023e
@oesteban oesteban mentioned this pull request Jul 25, 2025
Draft
@oesteban oesteban force-pushed the fix/itk-displacements-field-round-trip branch from 961a020 to b807020 Compare July 26, 2025 06:11
@oesteban
Copy link
Collaborator Author

It doesn't look this transposition is the right answer (pictures extracted within the test_densefield_map_vs_ants test in this PR, which uses @feilong's 01_ants_t1_to_mniComposite_DisplacementFieldTransform.nii.gz:

Displacements with antsApplyTransformsToPoints:
Screenshot 2025-07-26 at 17 58 35

Displacements with nitransforms:
Screenshot 2025-07-26 at 17 58 22

@oesteban oesteban force-pushed the fix/itk-displacements-field-round-trip branch from b807020 to eee5a4e Compare July 27, 2025 07:16
@oesteban oesteban mentioned this pull request Aug 12, 2025
Merged
oesteban added a commit that referenced this pull request Aug 12, 2025
@oesteban
wip: disable tests that will be addressed by #266
f0822f9
@oesteban oesteban force-pushed the fix/itk-displacements-field-round-trip branch 3 times, most recently from 449d396 to 3aeb18e Compare August 13, 2025 07:00
oesteban added a commit that referenced this pull request Aug 13, 2025
@oesteban
wip: disable tests that will be addressed by #266
3097dfb
@oesteban oesteban force-pushed the fix/itk-displacements-field-round-trip branch 3 times, most recently from b20e1d9 to 1820103 Compare August 13, 2025 07:31
oesteban added a commit that referenced this pull request Aug 13, 2025
@oesteban
Merge pull request #274 from nipy/enh/extract-test-from-266
60168b2 
ENH: Add unit test on dense fields (extracted from #266)
@oesteban oesteban force-pushed the fix/itk-displacements-field-round-trip branch 3 times, most recently from 51672ed to 6d32a6f Compare August 13, 2025 08:59
oesteban and others added 6 commits August 13, 2025 11:02
@oesteban @feilong
fix: proper ordering of displacements fields when reading and writing
d624b2a 
Transposes data as suggested by @feilong when reading and writing
displacements fields.

Tests have been added to ensure round-trip consistency.

Related-to: #171.

Co-Authored-By: Feilong Ma <mafeilong@gmail.com>
@oesteban
fix: remove sign flip as it seems incorrect
b2a794b
@oesteban
enh: update tests accordingly
ae6c330
@oesteban
fix: write tests to avoid regressions
6a36577 
Resolves: #267.
@oesteban
fix: make nitransforms/tests/test_nonlinear.py::test_densefield_map_v…
c033fce 
…s_ants pass
@oesteban
enh: ITK's coordinate system deciphered
6d32a6f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oesteban @feilong