ENH: Collapse linear and nonlinear transforms chains #170
Conversation
oesteban
force-pushed
the
enh/89-collapse-nonlinear
branch
from
d25308d to
1102726
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #170 +/- ##
==========================================
- Coverage 94.88% 0.00% -94.89%
==========================================
Files 16 16
Lines 1859 1849 -10
Branches 201 0 -201
==========================================
- Hits 1764 0 -1764
- Misses 78 1849 +1771
+ Partials 17 0 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Apologies for the slow response. I'm worried that this is doing something backwards driven by a misinterpretation of ITK's H5, rather than correcting an internal representation. I suspect what needs to happen is reversing the order of ITK's list when we take it in. From an API perspective, it seems almost guaranteed to trip up users if Aff(m1) @ Aff(m2) != Aff(m1 @ m2).
| @@ -8,7 +8,6 @@ | |||
| ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ## | |||
| """Common interface for transforms.""" | |||
| from collections.abc import Iterable | |||
There was a problem hiding this comment.
| from collections.abc import Iterable | |
| from collections.abc import Iterable | |
| from functools import reduce | |
| import operator as op |
| retval = self.transforms[-1] | ||
| for xfm in reversed(self.transforms[:-1]): | ||
| retval = xfm @ retval | ||
| return retval |
There was a problem hiding this comment.
I feel like it would be more intuitive to swap the arguments of the @ than to reverse the order of the list:
| 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):
| retval = self.transforms[-1] | |
| for xfm in reversed(self.transforms[:-1]): | |
| retval = xfm @ retval | |
| return retval | |
| return reduce(op.matmul, self.transforms) |
| assert composed.reference is None | ||
| assert composed == nitl.Affine(mat1.dot(mat2)) | ||
| assert composed == nitl.Affine(mat2 @ mat1) |
There was a problem hiding this comment.
Can you add comment on why we should expect Affine(mat1) @ Affine(mat2) == Affine(mat2 @ mat1)? This seems counterintuitive.
Very undertested, but currently there is a test that uses a "collapsed" transform on an ITK's .h5 file with one affine and one nonlinear. BSpline transforms not currently supported. Resolves #89.
oesteban
force-pushed
the
enh/89-collapse-nonlinear
branch
from
1102726 to
fbc9228
Compare
|
I will be resuscitating this one over this week. Thanks for your patience! |
|
@mattcieslak also this (I'm remembering as I go) :D |
Very undertested, but there is a new test that uses a "collapsed" transform from an ITK's .h5 file with one affine and one nonlinear (and it works).
BSpline transforms are not currently supported.
Related: #167, #169.
Resolves #89.