Skip to content
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.

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

ENH: Collapse linear and nonlinear transforms chains #170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions nitransforms/linear.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,19 +123,17 @@ def __matmul__(self, b):
True

>>> xfm1 = Affine([[1, 0, 0, 4], [0, 1, 0, 0], [0, 0, 1, 0], [0, 0, 0, 1]])
>>> xfm1 @ np.eye(4) == xfm1
>>> xfm1 @ Affine() == xfm1
True

"""
if not isinstance(b, self.__class__):
_b = self.__class__(b)
else:
_b = b
if isinstance(b, self.__class__):
return self.__class__(
b.matrix @ self.matrix,
reference=b.reference,
)

retval = self.__class__(self.matrix.dot(_b.matrix))
if _b.reference:
retval.reference = _b.reference
return retval
return b @ self

@property
def matrix(self):
Expand Down
16 changes: 7 additions & 9 deletions nitransforms/manip.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ##
"""Common interface for transforms."""
from collections.abc import Iterable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from collections.abc import Iterable
from collections.abc import Iterable
from functools import reduce
import operator as op

import numpy as np

from .base import (
TransformBase,
Expand Down Expand Up @@ -140,17 +139,17 @@ def map(self, x, inverse=False):

return x

def asaffine(self, indices=None):
def collapse(self):
"""
Combine a succession of linear transforms into one.
Combine a succession of transforms into one.

Example
------
>>> chain = TransformChain(transforms=[
... Affine.from_matvec(vec=(2, -10, 3)),
... Affine.from_matvec(vec=(-2, 10, -3)),
... ])
>>> chain.asaffine()
>>> chain.collapse()
array([[1., 0., 0., 0.],
[0., 1., 0., 0.],
[0., 0., 1., 0.],
Expand All @@ -160,15 +159,15 @@ def asaffine(self, indices=None):
... Affine.from_matvec(vec=(1, 2, 3)),
... Affine.from_matvec(mat=[[0, 1, 0], [0, 0, 1], [1, 0, 0]]),
... ])
>>> chain.asaffine()
>>> chain.collapse()
array([[0., 1., 0., 2.],
[0., 0., 1., 3.],
[1., 0., 0., 1.],
[0., 0., 0., 1.]])

>>> np.allclose(
... chain.map((4, -2, 1)),
... chain.asaffine().map((4, -2, 1)),
... chain.collapse().map((4, -2, 1)),
... )
True

Expand All @@ -178,9 +177,8 @@ def asaffine(self, indices=None):
The indices of the values to extract.

"""
affines = self.transforms if indices is None else np.take(self.transforms, indices)
retval = affines[0]
for xfm in affines[1:]:
retval = self.transforms[-1]
for xfm in reversed(self.transforms[:-1]):
retval = xfm @ retval
return retval
Comment on lines +180 to 183
Copy link
Member

Choose a reason for hiding this comment

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

I feel like it would be more intuitive to swap the arguments of the @ than to reverse the order of the list:

Suggested change
retval = self.transforms[-1]
for xfm in reversed(self.transforms[:-1]):
retval = xfm @ retval
return retval
retval = affines[0]
for xfm in affines[1:]:
retval = retval @ xfm
return retval

But we can also just use a reduce (I've added the imports above if you want to go this way):

Suggested change
retval = self.transforms[-1]
for xfm in reversed(self.transforms[:-1]):
retval = xfm @ retval
return retval
return reduce(op.matmul, self.transforms)


Expand Down
6 changes: 3 additions & 3 deletions nitransforms/tests/test_linear.py
Original file line number Diff line number Diff line change
Expand Up @@ -372,10 +372,10 @@ def test_mulmat_operator(testdata_path):
mat2 = from_matvec(np.eye(3), (4, 2, -1))
aff = nitl.Affine(mat1, reference=ref)

composed = aff @ mat2
composed = aff @ nitl.Affine(mat2)
assert composed.reference is None
assert composed == nitl.Affine(mat1.dot(mat2))
assert composed == nitl.Affine(mat2 @ mat1)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add comment on why we should expect Affine(mat1) @ Affine(mat2) == Affine(mat2 @ mat1)? This seems counterintuitive.


composed = nitl.Affine(mat2) @ aff
assert composed.reference == aff.reference
assert composed == nitl.Affine(mat2.dot(mat1), reference=ref)
assert composed == nitl.Affine(mat1 @ mat2, reference=ref)
8 changes: 7 additions & 1 deletion nitransforms/tests/test_manip.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ def test_itk_h5(tmp_path, testdata_path):
# A certain tolerance is necessary because of resampling at borders
assert (np.abs(diff) > 1e-3).sum() / diff.size < RMSE_TOL

col_moved = xfm.collapse().apply(img_fname, order=0)
col_moved.to_filename("nt_collapse_resampled.nii.gz")
diff = sw_moved.get_fdata() - col_moved.get_fdata()
# A certain tolerance is necessary because of resampling at borders
assert (np.abs(diff) > 1e-3).sum() / diff.size < RMSE_TOL


@pytest.mark.parametrize("ext0", ["lta", "tfm"])
@pytest.mark.parametrize("ext1", ["lta", "tfm"])
Expand All @@ -81,7 +87,7 @@ def test_collapse_affines(tmp_path, data_path, ext0, ext1, ext2):
]
)
assert np.allclose(
chain.asaffine().matrix,
chain.collapse().matrix,
Affine.from_filename(
data_path / "regressions" / f"from-fsnative_to-bold_mode-image.{ext2}",
fmt=f"{FMT[ext2]}",
Expand Down