FIX: Misinterpretation of voxel ordering in LTAs #129
Conversation
The structarray was used directly and the extra axis actually changed the order of operations when the direction cosines were scaled by the voxel sizes. A new test case has been added for which this error was apparent. This bug caused nipreps/fmriprep#2307, nipreps/fmriprep#2393, and nipreps/fmriprep#2410. nipreps/fmriprep#2444 just works around the problem by using ``lta_convert`` instead of *NiTransforms*. The ``lta_convert`` tool can be now dropped. Resolves: #125
|
Hello @oesteban! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-07-21 14:31:36 UTC |
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
==========================================
- Coverage 98.89% 98.88% -0.01%
==========================================
Files 12 12
Lines 1084 1081 -3
Branches 138 138
==========================================
- Hits 1072 1069 -3
Misses 6 6
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
oesteban
force-pushed
the
fix/lta-concatenation
branch
from
78bb9c5 to
f615827
Compare
Co-authored-by: Chris Markiewicz <[email protected]>
oesteban
force-pushed
the
fix/lta-concatenation
branch
from
f615827 to
1d1d202
Compare
| oracle = np.loadtxt(StringIO("""\ | ||
| -3.0000 0.0000 -0.0000 91.3027 | ||
| -0.0000 2.0575 -2.9111 -25.5251 | ||
| 0.0000 2.1833 2.7433 -105.0820 | ||
| 0.0000 0.0000 0.0000 1.0000""")) | ||
|
|
||
| lta_text = "\n".join( | ||
| (data_path / "bold-to-t1w.lta").read_text().splitlines()[13:21] | ||
| ) | ||
| r2r = VG.from_string(lta_text) | ||
| assert np.allclose(r2r.as_affine(), oracle, rtol=1e-4) |
There was a problem hiding this comment.
AFAICT this test doesn't fail before this PR. What exactly is the edge case we're fixing here?
There was a problem hiding this comment.
Yes, this failed before this PR.
There was a problem hiding this comment.
The test breaks current master without the patch proposed -> https://github.com/poldracklab/nitransforms/compare/fix/vg-lta-test (EDIT: also https://github.com/poldracklab/nitransforms/runs/3121490781)
|
Good to go? Do you feel the comments have been successfully addressed? |
|
Sorry, just getting back to this this morning. Give me one hour. |
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
The structarray was used directly and the extra axis actually changed the order of operations when the direction cosines were scaled by the voxel sizes.
A new test case has been added for which this error was apparent. This bug caused nipreps/fmriprep#2307, nipreps/fmriprep#2393, and nipreps/fmriprep#2410. nipreps/fmriprep#2444 just works around the problem by using
lta_convertinstead of NiTransforms. Thelta_converttool can be now dropped.Resolves: #125