Skip to content

Image Rotation by shearing - #1347

Merged
garrettwrong merged 21 commits into
developfrom
fasrot
Dec 12, 2025
Merged

Image Rotation by shearing#1347
garrettwrong merged 21 commits into
developfrom
fasrot

Conversation

@garrettwrong

Copy link
Copy Markdown
Collaborator

Port of the fastrotate MATLAB code. Adds masking and exposes as a configurable method Image.rotate as discussed.

The method is not optimized, but we don't have a strong use/application case for that ATM. It did not meaningfully improve recons.

Still WIP.

@garrettwrong garrettwrong self-assigned this Dec 4, 2025
@garrettwrong garrettwrong added the enhancement New feature or request label Dec 4, 2025
@codecov

codecov Bot commented Dec 4, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.42424% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.74%. Comparing base (8cb721c) to head (d351fea).
⚠️ Report is 21 commits behind head on develop.

Files with missing lines Patch % Lines
src/aspire/image/image.py 66.66% 9 Missing ⚠️
src/aspire/image/rotation.py 99.03% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1347      +/-   ##
===========================================
+ Coverage    90.72%   90.74%   +0.02%     
===========================================
  Files          135      136       +1     
  Lines        14558    14688     +130     
===========================================
+ Hits         13208    13329     +121     
- Misses        1350     1359       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Looks good. Just one-ish thing.

Comment thread src/aspire/image/rotation.py
Comment thread src/aspire/image/rotation.py Outdated
@garrettwrong
garrettwrong requested a review from j-c-c December 10, 2025 15:20
@garrettwrong

garrettwrong commented Dec 10, 2025

Copy link
Copy Markdown
Collaborator Author

Looks like CI is now failing for something new and exciting, but unrelated. Classic December :D.

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

Looks like there's still a problem for stacks provided directly to the sp_rotate method. See my response to the previous review comment.

@garrettwrong

Copy link
Copy Markdown
Collaborator Author

Yeah, that was an unintended use case. The docstring implies the intended use is for a single rotation :param theta: Rotation in ccw radians.

Its like the multidim handling. I just happened to write code that worked for it... well, In this case, almost worked, for additional cases that were not the application...

Alas, it should work for these additional cases now. There is no reason/benefit to use these wrappers in this way though. If I missed one just let me know and I'll add it, thanks.

j-c-c
j-c-c previously approved these changes Dec 11, 2025

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

Looks good!

@garrettwrong
garrettwrong marked this pull request as ready for review December 11, 2025 20:35
@garrettwrong
garrettwrong requested a review from janden as a code owner December 11, 2025 20:35
janden
janden previously approved these changes Dec 12, 2025
Comment thread src/aspire/image/rotation.py Outdated

:param images: (n , px, px) array of image data
:param theta: rotation angle in radians
:param M: optional precomputed shearing table

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.

If this is supplied, theta is ignored, right? We should mention that if that's the case.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added check and test for this in 5091f33

@garrettwrong
garrettwrong dismissed stale reviews from janden and j-c-c via 5091f33 December 12, 2025 14:48
@garrettwrong
garrettwrong requested a review from j-c-c December 12, 2025 14:49

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

Looks good!

@garrettwrong
garrettwrong merged commit 55d7d92 into develop Dec 12, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants