-
Notifications
You must be signed in to change notification settings - Fork 25
Fast (Polar) Rotational Alignment #1262
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1262 +/- ##
===========================================
- Coverage 90.60% 90.52% -0.09%
===========================================
Files 132 132
Lines 14181 14285 +104
===========================================
+ Hits 12849 12931 +82
- Misses 1332 1354 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
88fd522
to
e3493c3
Compare
Just some updates. I'm still testing this without interpolation on 10073 and 10028 recons and manually validating the class averages in a notebook. So far I had to fix a few things in the PR, but looking good now. I've intentionally left the interp/optimizer stuff unpolished to discuss whether its worth continuing on that at this time. |
Posting code snippet of interpolation/optimizer for potential future use. I'll move forward with removing it as discussed in out meeting.
|
0505dad
to
a6d5b46
Compare
I'm going to begin testing this for with shifts. While I'm doing that can open for initial review. |
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 a couple things.
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.
LGTM!
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.
Very cool! Some questions here and there, but nothing too big.
@@ -4,6 +4,7 @@ | |||
BFRAverager2D, | |||
BFSRAverager2D, | |||
BFSReddyChatterjiAverager2D, | |||
BFTAverager2D, |
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 we're keeping with the naming scheme, shouldn't it be BFS
(for shifts)? There are probably better names for all of these, but that's a discussion for another day.
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.
Haha, maybe, I was actually going to migrate towards the names you prefer. Mainly because there is a paper that can be referenced for their definitions. We can deprecate/rename some of the other ones once this one is settled.
I thought I at least got this name correct 😇 .
pfB = fft.fft(pfB, axis=-2) | ||
# Tabulate elements of pfA cross pfB.conj() using broadcast multiply | ||
x = xp.expand_dims(pfA, 1) * xp.expand_dims(pfB.conj(), 0) | ||
angular = xp.sum(xp.abs(fft.ifft2(x)), axis=-1) # sum all radial contributions |
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.
Are we sure about the abs
here? Also, adding radial weighting here didn't help, was that it?
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.
I am not sure. I just checked via a breakpoint on the unit test. Using fft.ifft2(x).real
in the sum does not appear to work as well as what I have with the abs
magnitude (test case fails to be close to the reference angle). The imaginary values are approximately half the magnitude of the real values here. Both are small, which might be a problem....
I'll try it again with weighting, after I hit the other review comments. Perhaps they must both be in place, or I misunderstood what we talked about.
# Tabulate elements of pfA cross pfB.conj() using broadcast multiply
pfX = xp.expand_dims(pfA, 1) * xp.expand_dims(pfB.conj(), 0)
X = fft.ifft2(pfX)
breakpoint()
# Check imaginary component (should be small)
max_imag_err = xp.max(X.imag)
if max_imag_err > 1e-8: # a guess for now
logger.warning(f"Imaginary component {max_imag_err} larger than expected")
# Sum all radial contributions
angular = xp.sum(X.real, axis=-1)
pytest tests/test_averager2d.py -k BFTAverager2DTestCase
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.
I've tried quite a few things here now, and none seem to work better than this implementation so far. 🤷♂️ . I'm thinking we move forward and can revisit with a patch if needed. (Similar for the weighting issue, which I made some progress at least identifying the source....).
|
||
# Shift the base, original_image[0], for each shift in this batch | ||
# Note this includes shifting for the zero case | ||
template_images[:bs] = xp.asarray( |
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.
This can potentially be sped up by polar Fourier-transforming the base images once, then applying phase shifts (elementwise multiplication in Fourier) on those polar Fourier representations.
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.
Sorry for the hold up here. My first pass at this also didn't work well. Still working on it.
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.
I was able to figure this one out. For some reason I think the Polar freq grids are XY swapped. I'll make an issue about checking that, but for now I was able to create PolarFT.shift
and a unit test as things are. I will replace the shifting in averager2d.py
next for the speedup.
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.
I've added the PolarFT.shift with the required broadcasting and gpu support. On large problems we get about 5-10% speedup, but I had to give up some masking which may have been beneficial in practice (I'll see in the recon tests later).
Although I'm somewhat unsatisfied this isn't "just right", the PR has been stalled too long and I just found some major issues elsewhere I need to deal with.... |
Implements rotational alignment using polar cross correlation and brute force translations.
This required minor modification to
Image
andPolarFT
.I still need to cleanup several things and add fine interpolation.