ENH: Collapse linear and nonlinear transforms chains#170
Conversation
oesteban
force-pushed
the
enh/89-collapse-nonlinear
branch
from
d25308d to
1102726
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #170 +/- ##
==========================================
- Coverage 98.59% 0.00% -98.60%
==========================================
Files 13 16 +3
Lines 1279 1849 +570
Branches 184 0 -184
==========================================
- Hits 1261 0 -1261
- Misses 10 1849 +1839
+ Partials 8 0 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
effigies
left a comment
effigies
left a comment
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) |
| composed = aff @ nitl.Affine(mat2) | ||
| 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 |
…eport Configure coverage to omit tests
…oject.toml Move flake8 config into pyproject
…neartransformsmapping ENH: Loading of X5 (linear) transforms
TST: Refactor io/lta to reduce one partial line
…ling-issue FIX: Broken 4D resampling
MAINT: Increase coverage by testing edge cases and adding docstrings
2 participants
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.