ENH: AFNI - Enable conversion to RAS+ of affine transforms #49

oesteban · 2019-11-12T15:02:10Z

Comments on how to de-duplicate code very welcome :)

Now, these are not xfailing:

nitransforms/tests/test_io.py::test_Linear_common[afni-RAS] XPASS        [ 21%]
nitransforms/tests/test_io.py::test_Linear_common[afni-LAS] XPASS        [ 22%]
nitransforms/tests/test_io.py::test_Linear_common[afni-LPS] XPASS        [ 23%]

codecov-io · 2019-11-12T15:15:56Z

Codecov Report

Merging #49 into master will decrease coverage by 1.09%.
The diff coverage is 60%.

@@            Coverage Diff            @@
##           master      #49     +/-   ##
=========================================
- Coverage   98.76%   97.67%   -1.1%     
=========================================
  Files          10       10             
  Lines         810      817      +7     
  Branches      106      104      -2     
=========================================
- Hits          800      798      -2     
- Misses          6       13      +7     
- Partials        4        6      +2

Flag	Coverage Δ
#unittests	`97.67% <60%> (-1.1%)`	⬇️

Impacted Files	Coverage Δ
nitransforms/io/afni.py	`89.53% <60%> (-9.17%)`	⬇️
nitransforms/base.py	`98.8% <0%> (-0.6%)`	⬇️
nitransforms/io/itk.py	`100% <0%> (ø)`	⬆️
nitransforms/io/base.py	`100% <0%> (ø)`	⬆️

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 0ec1eb3...710f077. Read the comment docs.

mgxd

would be nice to unifying the reference / moving oblique calculation, maybe something like:

def from_oblique(image, itype):
    M = image.affine
    if itype == 'reference':
         A = shape_zoom_affine(image.shape, voxel_sizes(M), x_flip=False, y_flip=False)
         aff = M.dot(np.linalg.inv(A)).dot(LPS)
    elif itype == 'moving':
         A = shape_zoom_affine(image.shape, voxel_sizes(M), x_flip=True, y_flip=True)
         aff = A.dot(np.linalg.inv(M))
    return aff

mgxd · 2019-11-12T15:56:00Z

nitransforms/io/afni.py

+            moving = reference
+
+        if reference is None:
+            warnings.warn('At least, the reference image should be set.')


do we want to just warn or error here?

mgxd · 2019-11-12T16:08:39Z

nitransforms/io/afni.py

+        post = np.linalg.inv(post)
+
+        # swapaxes is necessary, as axis 0 encodes series of transforms
+        ras = self.structarr['parameters'].copy()


is copy necessary here?

mgxd · 2019-11-12T16:09:12Z

nitransforms/io/afni.py

@@ -18,6 +19,39 @@ def __str__(self):
        param = self.structarr['parameters']
        return '\t'.join(['%g' % p for p in param[:3, :].reshape(-1)])

+    def to_ras(self, moving=None, reference=None):
+        """Convert to RAS+ coordinate system."""
+        pre = LPS.copy()


just to simplify initialization

Suggested change

pre = LPS.copy()

pre = post = LPS.copy()

Close nipy#45, reduce some xfails (nipy#40)

pull-assistant · 2019-11-15T18:46:00Z

Best reviewed: commit by commit

Optimal code review plan (2 warnings, 2 commits squashed)

ENH: Enable conversion to RAS+ of affine transforms

nitransforms/io/afni.py 69% changes removed in enh: working on afni...

fix: first stab at AFNI obliques

enh: working on afni's affine oblique

...ransforms/tests/test_linear.py 67% changes removed in fix: testing with ca...

fix: does AFNI use t... ... fix: rethink ras2afn...

Squashed 2 commits:

fix: does AFNI use the translation part of WARPDRIVE_FOR?

fix: rethink ras2afni and afni2ras

fix: testing with cat_matvec-built matrices

Powered by Pull Assistant. Last update a21bbd5 ... b8a715f. Read the comment docs.

pep8speaks · 2019-11-15T22:55:05Z

Hello @oesteban! Thanks for updating this PR.

In the file nitransforms/tests/test_linear.py:

Line 112:1: E302 expected 2 blank lines, found 1

oesteban · 2022-02-16T17:46:04Z

Superseded by #117

oesteban requested a review from mgxd November 12, 2019 15:02

mgxd reviewed Nov 12, 2019

View reviewed changes

oesteban added 4 commits November 14, 2019 13:00

fix: first stab at AFNI obliques

ad584e4

Close nipy#45, reduce some xfails (nipy#40)

ENH: Enable conversion to RAS+ of affine transforms

a21bbd5

enh: working on afni's affine oblique

da00997

fix: does AFNI use the translation part of WARPDRIVE_FOR?

1b1958a

oesteban force-pushed the fix/enable-afni-to-ras branch from 2ec1f10 to 1b1958a Compare November 15, 2019 18:45

oesteban added 2 commits November 15, 2019 13:49

fix: rethink ras2afni and afni2ras

710f077

fix: testing with cat_matvec-built matrices

b8a715f

oesteban closed this Feb 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: AFNI - Enable conversion to RAS+ of affine transforms #49

ENH: AFNI - Enable conversion to RAS+ of affine transforms #49

oesteban commented Nov 12, 2019 •

edited

Loading

codecov-io commented Nov 12, 2019 •

edited

Loading

mgxd left a comment

mgxd Nov 12, 2019

mgxd Nov 12, 2019

mgxd Nov 12, 2019

pull-assistant bot commented Nov 15, 2019 •

edited

Loading

pep8speaks commented Nov 15, 2019

oesteban commented Feb 16, 2022

ENH: AFNI - Enable conversion to RAS+ of affine transforms #49

ENH: AFNI - Enable conversion to RAS+ of affine transforms #49

Conversation

oesteban commented Nov 12, 2019 • edited Loading

codecov-io commented Nov 12, 2019 • edited Loading

Codecov Report

mgxd left a comment

Choose a reason for hiding this comment

mgxd Nov 12, 2019

Choose a reason for hiding this comment

mgxd Nov 12, 2019

Choose a reason for hiding this comment

mgxd Nov 12, 2019

Choose a reason for hiding this comment

pull-assistant bot commented Nov 15, 2019 • edited Loading

pep8speaks commented Nov 15, 2019

oesteban commented Feb 16, 2022

oesteban commented Nov 12, 2019 •

edited

Loading

codecov-io commented Nov 12, 2019 •

edited

Loading

pull-assistant bot commented Nov 15, 2019 •

edited

Loading