Skip to content

Improve affine decomposition - #1186

Merged
LucaMarconato merged 2 commits into
mainfrom
improve/affine-decomposition
Aug 25, 2026
Merged

Improve affine decomposition#1186
LucaMarconato merged 2 commits into
mainfrom
improve/affine-decomposition

Conversation

@LucaMarconato

@LucaMarconato LucaMarconato commented Aug 20, 2026

Copy link
Copy Markdown
Member

Claude generated PR description, manually verified

Port the RQ-based decomposition algorithm from Spatial-Innovation-Team/transfo
to replace the old 2D-only, theta-based rotation logic in
_decompose_transformation. The new algorithm is dimension-generic, warns on
ill-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 z axis
and the c channel are now decomposed like any other axis, and axes are
allowed to come out in a different order between input and output (checked
by axis set rather than exact tuple order).

Split _decompose_transformation into _decompose_transformation_simple and
_decompose_transformation_full, each returning a fixed-size tuple of the
component transformations instead of a Sequence, so callers get a
concretely-typed tuple (2 vs. 5 components) rather than a Union.

Reorganize the tests into TestSimpleDecomposition/TestFullDecomposition
classes, consolidating round-trip coverage into a single
DECOMPOSE_TRANSFORMATION_CASES table (with descriptive ids) shared by both
classes, and keeping only the checks that can't be expressed as a
matrix-in/reconstructed-matrix-out case (warnings, component types/arity,
algorithmic invariants).

@LucaMarconato
LucaMarconato changed the base branch from main to fix/bounding-box-query-axes-dependent-results August 20, 2026 13:07
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.39%. Comparing base (8731a46) to head (eb66dd5).

Files with missing lines Patch % Lines
src/spatialdata/transformations/transformations.py 98.14% 1 Missing ⚠️
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     
Files with missing lines Coverage Δ
src/spatialdata/transformations/transformations.py 90.79% <98.14%> (-0.29%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LucaMarconato

Copy link
Copy Markdown
Member Author

@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.

@LucaMarconato
LucaMarconato changed the base branch from fix/bounding-box-query-axes-dependent-results to main August 20, 2026 13:27
@LucaMarconato
LucaMarconato force-pushed the improve/affine-decomposition branch from ed6769d to 2f10ecd Compare August 20, 2026 13:28
@ajkswamy
ajkswamy force-pushed the improve/affine-decomposition branch from 2f10ecd to 2270e60 Compare August 21, 2026 09:38

@Tomaz-Vieira Tomaz-Vieira left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread src/spatialdata/transformations/transformations.py Outdated
Comment thread src/spatialdata/transformations/transformations.py Outdated
Comment thread src/spatialdata/transformations/transformations.py Outdated
Comment thread src/spatialdata/transformations/transformations.py Outdated
Comment thread src/spatialdata/transformations/transformations.py Outdated
Comment thread src/spatialdata/transformations/transformations.py Outdated
@LucaMarconato

Copy link
Copy Markdown
Member Author

@Tomaz-Vieira thanks for the review, really helpful! I have addressed all the review comments. I have also renamed the two functions form decompose_affine_simple/full to:

  • _decompose_into_linear_and_translation
  • _decompose_into_5_simple_transformations (not sure if it's a good name).

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.
@LucaMarconato
LucaMarconato force-pushed the improve/affine-decomposition branch from 11ff7cc to eb66dd5 Compare August 25, 2026 12:25
@LucaMarconato

Copy link
Copy Markdown
Member Author

Had to do a manual rebase as I stacked PRs complained about the merge commit.

@LucaMarconato
LucaMarconato merged commit 908ae5e into main Aug 25, 2026
10 checks passed
@LucaMarconato
LucaMarconato deleted the improve/affine-decomposition branch August 25, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants