Improve affine decomposition - #1186
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1186 +/- ##
==========================================
- Coverage 92.41% 92.40% -0.02%
==========================================
Files 52 52
Lines 7875 7870 -5
==========================================
- Hits 7278 7272 -6
- Misses 597 598 +1
🚀 New features to boost your workflow:
|
|
@Tomaz-Vieira can you please review? Also, the GitHub diff view is not the most useful for this PR since the code is now split into functions. I suggest to use a standard diff view, so that most of the code is easily shown as untouched. |
ed6769d to
2f10ecd
Compare
ported from transfo: supporting z and c; improved order of returned transformations also: split into simple/full; changed return type to tuples; supporting permutation of input/output axes for the transformation
2f10ecd to
2270e60
Compare
Tomaz-Vieira
left a comment
Tomaz-Vieira
left a comment
There was a problem hiding this comment.
The decomposition is a very nice maneuver =)
My comments are more on the code organization side of things; in particular, I argue for expressing the requirements of the functions more clearly, and leaving most of the responsibility of meeting those requirements to the callers of the functions.
| translation_part = matrix[:-1, -1] | ||
| linear_part = matrix[:-1, :-1] |
There was a problem hiding this comment.
Could these be fields/properties/methods of Affine? Like my_affine.linear and my_affine.translation?
|
|
||
| Returns | ||
| ------- | ||
| A tuple ``(linear, translation)``, applied in this order (``linear`` first), whose composition equals |
There was a problem hiding this comment.
I think the intention was something like
A tuple
(linear, translation), whose composition equalstransformation, applied in the following order:
|
|
||
|
|
||
| def _decompose_transformation_simple( | ||
| transformation: BaseTransformation, input_axes: tuple[ValidAxis_t, ...] |
There was a problem hiding this comment.
Could we change parameters to just transformation: Affine? In fact, could we even make this a method of Affine 🤔 ?
The docstring for _validate_square_affine_for_decomposition even says "It is assumed to be of a type that can be represented as a single affine", so we might as well require proof of that, by asking directly for an instance of Affine. Then we can just check that it's square, and use exactly transformation.input_axes and transformation.output_axes rather than another input_axes argument and the fallible _get_current_output_axes. This way we move a bunch of errors out of this function, plus we don't have this indirection on the param requirements, which requires the reader to go read _validate_square_affine_for_decomposition
| """ | ||
| matrix, translation_part, linear_part = _validate_square_affine_for_decomposition(transformation, input_axes) | ||
|
|
||
| linear = _compose_affine_from_linear_and_translation( |
There was a problem hiding this comment.
I think this part would also be eliminated by requiring the transformation param to be only a square transformation: Affine
| ) | ||
| translation = Translation(translation_part, axes=input_axes) | ||
|
|
||
| check_m = Sequence([linear, translation]).to_affine_matrix(input_axes=input_axes, output_axes=input_axes) |
There was a problem hiding this comment.
To be super pedantic, this could be moved into an if __debug__, since this value is only created for the sake of the assertion
|
|
||
|
|
||
| def _decompose_transformation_full( | ||
| transformation: BaseTransformation, input_axes: tuple[ValidAxis_t, ...] |
There was a problem hiding this comment.
same argument for at last making transformation be of type Affine
2 participants
Claude generated PR description, manually verified
Port the RQ-based decomposition algorithm from
Spatial-Innovation-Team/transfoto replace the old 2D-only, theta-based rotation logic in
_decompose_transformation. The new algorithm is dimension-generic, warns onill-conditioned linear parts, and encodes the reflection as a single
axis flip instead of an arbitrary diagonal matrix.
Drop the spatialdata-specific restrictions that no longer apply: the
zaxisand the
cchannel are now decomposed like any other axis, and axes areallowed to come out in a different order between input and output (checked
by axis set rather than exact tuple order).
Split
_decompose_transformationinto_decompose_transformation_simpleand_decompose_transformation_full, each returning a fixed-size tuple of thecomponent transformations instead of a
Sequence, so callers get aconcretely-typed tuple (2 vs. 5 components) rather than a
Union.Reorganize the tests into
TestSimpleDecomposition/TestFullDecompositionclasses, consolidating round-trip coverage into a single
DECOMPOSE_TRANSFORMATION_CASEStable (with descriptive ids) shared by bothclasses, and keeping only the checks that can't be expressed as a
matrix-in/reconstructed-matrix-out case (warnings, component types/arity,
algorithmic invariants).