Skip to content

Conversation

@garrettwrong
Copy link
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
Copy link

codecov bot commented Dec 4, 2025

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.

Copy link
Collaborator

@j-c-c j-c-c left a comment

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.

@garrettwrong garrettwrong requested a review from j-c-c December 10, 2025 15:20
@garrettwrong
Copy link
Collaborator Author

garrettwrong commented Dec 10, 2025

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

Copy link
Collaborator

@j-c-c j-c-c left a comment

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
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
Copy link
Collaborator

@j-c-c j-c-c left a comment

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

:param images: (n , px, px) array of image data
:param theta: rotation angle in radians
:param M: optional precomputed shearing table
Copy link
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
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
Copy link
Collaborator

@j-c-c j-c-c left a comment

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.

4 participants