FIX: Implement AFNI's deoblique operations

[oesteban]

Finalize the interpretation in I/O of AFNI's linear matrices when reference
or moving data are oblique w.r.t. the canonical axes.

Resolves: #45.

[codecov-commenter]

Codecov Report

Merging #117 (4d4e3d9) into master (6fbcbc1) will increase coverage by 0.70%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   97.88%   98.59%   +0.70%     
==========================================
  Files          13       13              
  Lines        1184     1208      +24     
  Branches      183      184       +1     
==========================================
+ Hits         1159     1191      +32     
+ Misses         18       10       -8     
  Partials        7        7              

Flag	Coverage Δ
travis	96.77% <52.08%> (-1.04%)	⬇️
unittests	98.54% <95.83%> (+0.72%)	⬆️

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

Impacted Files	Coverage Δ
nitransforms/io/afni.py	98.29% <95.83%> (+9.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fbcbc1...4d4e3d9. Read the comment docs.

[pep8speaks]

Hello @oesteban! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-28 17:39:08 UTC

[oesteban]

Okay, this is as far as I can go for the moment (see afni/afni#353). I'm going to focus on other things while I wait for more feedback from AFNI folks.

/cc @effigies @matthew-brett

[effigies]

The notebooks don't run all the way through. Was hoping to use them to guide my review.

[oesteban]

Ouch, yes, I think they are not necessary anymore but I agree they are useful to understand some of the tests. Will try to rework them a little for your review.

…

On Fri, Feb 25, 2022, 21:57 Chris Markiewicz ***@***.***> wrote: ***@***.**** commented on this pull request. The notebooks don't run all the way through. Was hoping to use them to guide my review. ------------------------------ In nitransforms/io/afni.py <#117 (comment)> : > + ras : :obj:`bool` + Whether output should be referrenced to AFNI's internal system (LPS+) or RAS+ Unused. ------------------------------ In docs/notebooks/01_preparing_images.ipynb <#117 (comment)> : > @@ -745,9 +757,11 @@ } ], "source": [ - "!3dAllineate -base someones_anatomy_rot.nii.gz -input someones_anatomy_rot.nii.gz -1Dmatrix_apply M.afni -prefix moved-afni.nii.gz -final NN\n", - "nb.load('moved-afni.nii.gz').orthoview()\n", - "moved.orthoview()" + "afnixfm = nt.io.afni.AFNILinearTransform.from_filename('M.afni')\n", + "xfm = nt.linear.Affine(afnixfm.to_ras(), reference=deobnii)\n", + "nt_with_afni = xfm.apply(plumb, order=0)\n", plumb is not defined. — Reply to this email directly, view it on GitHub <#117 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESDRVNQMTAWTPCG2GQUWTU47UKBANCNFSM47W2FZLQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[effigies]

If they're not necessary, that's fine. Just assumed that with their changes they were intended to curate the situation.

[oesteban]

I was thinking about removing the notebook 02 about AFNI, and rewriting 01 to focus on stuff from the io submodule.

…

On Fri, Feb 25, 2022, 22:46 Chris Markiewicz ***@***.***> wrote: If they're not necessary, that's fine. Just assumed that with their changes they were intended to curate the situation. — Reply to this email directly, view it on GitHub <#117 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESDRQ7GYA5VXYUKT2JZCTU472CNANCNFSM47W2FZLQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[oesteban]

Okay, I think now the notebooks are in much better shape. LMKWYT.

[oesteban]

I would imagine you are really up to your eyeballs @effigies - should I wait for review or can I go ahead and maybe you can catch up later when the integration with nibabel starts?

[effigies]

Was just starting to have a quick look before our call, and planned to finish up after. I'm okay if you want to go ahead and merge, but I did want to suggest that it might make sense to think of affines generally as T @ R_O @ R_C @ Z @ S where R_C is the cardinal rotation and R_O is an additional oblique rotation <90deg. I haven't thought through the details, so it might be too cumbersome, but it seems like this conceptualization could allow us to write code that is more similar across tools and make the specific deviations of AFNI clear in the algebra, rather than via special functions.

[effigies]

Okay, the code makes sense once I worked through the algebra, which I coded up as a docstring so others don't have to do it again. I read the tests with an eye to get the gist, but did not verify that all comments correctly described what was being done. This looks reasonable to me.