-
Notifications
You must be signed in to change notification settings - Fork 25
Image Rotation by shearing #1347
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
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
j-c-c
left a comment
There was a problem hiding this 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.
|
Looks like CI is now failing for something new and exciting, but unrelated. Classic December :D. |
j-c-c
left a comment
There was a problem hiding this 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.
|
Yeah, that was an unintended use case. The docstring implies the intended use is for a single rotation 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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
j-c-c
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
[skip ci]
[skip ci]
[skip ci]
[skip ci]
Port of the
fastrotateMATLAB code. Adds masking and exposes as a configurable methodImage.rotateas 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.