Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decompose affine into simpler transformations #327

Merged
merged 3 commits into from
Oct 28, 2023

Conversation

LucaMarconato
Copy link
Member

Helper function for @timtreis as asked in #314.

@LucaMarconato LucaMarconato linked an issue Jul 25, 2023 that may be closed by this pull request
@LucaMarconato
Copy link
Member Author

LucaMarconato commented Jul 25, 2023

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
Copy link
Member Author

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.

@LucaMarconato LucaMarconato requested a review from timtreis July 25, 2023 23:38
@LucaMarconato LucaMarconato changed the title implemented _decompose_transformation() decompose affine into simpler transformations Jul 25, 2023
@codecov
Copy link

codecov bot commented Jul 25, 2023

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

❗ 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
@@            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 Coverage Δ
src/spatialdata/transformations/transformations.py 92.36% <100.00%> (+0.87%) ⬆️

... and 13 files with indirect coverage changes

@LucaMarconato
Copy link
Member Author

LucaMarconato commented Jul 25, 2023

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
Copy link
Member Author

@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
Copy link
Member Author

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

@LucaMarconato LucaMarconato merged commit da7cc58 into main Oct 28, 2023
1 of 5 checks passed
@LucaMarconato LucaMarconato deleted the util/affine_decomposition branch October 28, 2023 19:26
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.

Dev feature: decompose affine transformation into its component
1 participant