Improve affine decomposition - #1186
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1186 +/- ##
==========================================
- Coverage 92.41% 92.39% -0.02%
==========================================
Files 52 52
Lines 7875 7868 -7
==========================================
- Hits 7278 7270 -8
- 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
2f10ecd to
2270e60
Compare
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.
|
@Tomaz-Vieira thanks for the review, really helpful! I have addressed all the review comments. I have also renamed the two functions form
I will merge as it is required for the bounding box PR; if we need to change something (like the name of the functions) we can do it in a follow up PR; they are private and unreleased anyway. |
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
Also, _decompose_affine_into_linear_and_translation becomes a pure validation method, as opposed to returning linear and translation (task now moved into Affine properties). Update tests accordingly.
11ff7cc to
eb66dd5
Compare
|
Had to do a manual rebase as I stacked PRs complained about the merge commit. |
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).