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.

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

FIX: Update implementation of ndim property of transforms #197

Merged
merged 14 commits into from
Apr 19, 2024
Merged

FIX: Update implementation of ndim property of transforms #197

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d865ec5
enh: outsource the apply function
jmarabotto Apr 5, 2024
d6dca85
sty: pacify flake8
jmarabotto Apr 5, 2024
e299dc1
sty: fix imports
jmarabotto Apr 5, 2024
ffd8a6d
fix: update many apply() calls
oesteban Apr 5, 2024
150c145
Implement ndim
Apr 18, 2024
f3529ce
ENH: Implement_ndim
Apr 18, 2024
3d40b93
enh: add a test on ndim
Apr 18, 2024
abe016e
fix: remove straneous files, revert changes from other branch
Apr 18, 2024
180ec18
enh: revert unrelated changes
Apr 18, 2024
c0e2a98
enh: revert unrelated changes
Apr 18, 2024
90e529b
enh: revert unrelated changes
Apr 18, 2024
bafd2ed
enh: revert unrelated changes
Apr 18, 2024
acb0736
Update nitransforms/tests/test_base.py
oesteban Apr 19, 2024
4a18200
Update nitransforms/linear.py
oesteban Apr 19, 2024
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
4 changes: 2 additions & 2 deletions nitransforms/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def __ne__(self, other):
class TransformBase:
"""Abstract image class to represent transforms."""

__slots__ = ("_reference",)
__slots__ = ("_reference", "_ndim",)

def __init__(self, reference=None):
"""Instantiate a transform."""
Expand Down Expand Up @@ -220,7 +220,7 @@ def reference(self, image):
@property
def ndim(self):
"""Access the dimensions of the reference space."""
return self.reference.ndim
raise TypeError("TransformBase has no dimensions")

def apply(
self,
Expand Down
5 changes: 5 additions & 0 deletions nitransforms/linear.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ def matrix(self):
"""Access the internal representation of this affine."""
return self._matrix

@property
def ndim(self):
"""Access the internal representation of this affine."""
return self._matrix.ndim + 1
Comment on lines +148 to +149
Copy link
Member

Choose a reason for hiding this comment

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

Surely -1? A 4x4 affine represents a transform in 3D space, and nothing intelligible I'm aware of in 5 dimensions.

Also this docstring is off.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, it's a 2D matrix, so this works out to 3, but does so in all cases. What you actually want is self._matrix.shape[0] - 1

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not work for 2D transforms. But it does work for >2D:

  • self._matrix is 4x4 in 3D transforms (+1)
  • self._matrix is tx4x4 in 4D transforms (3D + t)
  • self._matrix is tx4x4x2 in 5D transforms (3D + t + complex numbers).

5D could become "a thing" when we want to realign 3D+t images reconstructed with magnitude and phase (or complex). This looks more plausible than generalization to 2D images (2D, 2D+t, 2D+t+part).

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so we're not considering e.g., a 5x5 affine matrix for transforms of 4D spaces, but a collection of 4x4 affines for a collection of 3D volumes.

If you want to be really general, you could treat it as self._matrix.shape[0] + self._matrix.ndim - 3, which would handle collections of any number of ND affine spaces. But I'm okay keeping this simplification and assuming 3D volumes for now.

As an aside: I don't really understand why you would treat the real and imaginary parts of complex numbers as being an extra dimension in this context. The interpolation would need to account for the complex plane (though on first thought, independent interpolation of real and imaginary components should work...) , but I don't see why the transform would.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, so we're not considering e.g., a 5x5 affine matrix for transforms of 4D spaces, but a collection of 4x4 affines for a collection of 3D volumes.

Correct

I don't really understand why you would treat the real and imaginary parts of complex numbers as being an extra dimension in this context.

The fact that this ndim could be 5D does not mean that the transform should use all the dimensions. I agree you (in principle) don't want to interpolate in between imaginary and real part.

If you want to be really general, you could treat it as self._matrix.shape[0] + self._matrix.ndim - 3, which would handle collections of any number of ND affine spaces. But I'm okay keeping this simplification and assuming 3D volumes for now.

I think your implementation is better; why keep the simplification once you've written a better alternative down here?


def map(self, x, inverse=False):
oesteban marked this conversation as resolved.
Show resolved Hide resolved
r"""
Apply :math:`y = f(x)`.
Expand Down
12 changes: 10 additions & 2 deletions nitransforms/tests/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ def _to_hdf5(klass, x5_root):
# Test identity transform
xfm = TransformBase()
xfm.reference = fname
assert xfm.ndim == 3
with pytest.raises(TypeError):
_ = xfm.ndim
moved = xfm.apply(fname, order=0)
assert np.all(
imgdata == np.asanyarray(moved.dataobj, dtype=moved.get_data_dtype())
Expand All @@ -103,12 +104,19 @@ def _to_hdf5(klass, x5_root):
# Test identity transform - setting reference
xfm = TransformBase()
xfm.reference = fname
assert xfm.ndim == 3
with pytest.raises(TypeError):
_ = xfm.ndim
moved = xfm.apply(str(fname), reference=fname, order=0)
assert np.all(
imgdata == np.asanyarray(moved.dataobj, dtype=moved.get_data_dtype())
)

#Test ndim returned by affine
oesteban marked this conversation as resolved.
Show resolved Hide resolved
assert nitl.Affine().ndim == 3
assert nitl.LinearTransformsMapping(
[nitl.Affine(), nitl.Affine()]
).ndim == 4

# Test applying to Gifti
gii = nb.gifti.GiftiImage(
darrays=[
Expand Down
Loading