search

scverse / spatialdata

An open and interoperable data framework for spatial omics data
https://spatialdata.scverse.org/
BSD 3-Clause "New" or "Revised" License
183 stars 35 forks source link

decompose affine into simpler transformations #327

Closed LucaMarconato closed 7 months ago

LucaMarconato commented 10 months ago

Helper function for @timtreis as asked in https://github.com/scverse/spatialdata/issues/314.

LucaMarconato commented 10 months ago

Please check the docstring of the _decompose_transformation() for a detailed explanation. Please let me know if you need more use cases or a different type of representation (like combining "shear" and "rotation" into a unique affine matrix, or forcing "shear" having diagonal values all 1 done; things like these).

LucaMarconato commented 10 months ago

I added some tests, but at each run the function is also doing some additional lightweight checks before returning, so if there are bugs we catch them.

codecov[bot] commented 10 months ago

Codecov Report

Merging #327 (c0bca76) into main (b25ec12) will decrease coverage by 0.73%. Report is 1 commits behind head on main. The diff coverage is 100.00%.

:exclamation: Current head c0bca76 differs from pull request most recent head 44f80bf. Consider uploading reports for the commit 44f80bf to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #327 +/- ## ========================================== - Coverage 91.42% 90.69% -0.73% ========================================== Files 36 36 Lines 4849 4740 -109 ========================================== - Hits 4433 4299 -134 - Misses 416 441 +25 ``` | [Files](https://app.codecov.io/gh/scverse/spatialdata/pull/327?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse) | Coverage Δ | | |---|---|---| | [src/spatialdata/transformations/transformations.py](https://app.codecov.io/gh/scverse/spatialdata/pull/327?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse#diff-c3JjL3NwYXRpYWxkYXRhL3RyYW5zZm9ybWF0aW9ucy90cmFuc2Zvcm1hdGlvbnMucHk=) | `92.36% <100.00%> (+0.87%)` | :arrow_up: | ... and [13 files with indirect coverage changes](https://app.codecov.io/gh/scverse/spatialdata/pull/327/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse)
LucaMarconato commented 10 months ago

Please also have a look at the much simpler _decompose_affine_into_linear_and_translation(). For the new transformations refactoring I will need the new function of this PR because I want to separate operations on the coordinates and operations on the raw data that needs to be committed. But for plotting this PR may be overkill (or I may need to make a simpler function).

LucaMarconato commented 10 months ago

@timtreis I made some changes, I added the parameter simple_decomposition to _decompose_transformation() and I believe that this is what you need (not _decompose_affine_into_linear_and_translation()). Thought about it and the original decomposition (simple_decomposition=False`) is probably not useful for matplotlib plotting.

LucaMarconato commented 7 months ago

Other PR have more priorities for code review. Gonna merge since the PR is thoroughly tested and since I am going to test it manually again when refactoring the transformations using this code. CC @kevinyamauchi @giovp