Closed ebezzam closed 5 months ago
Awesome work, thank you @ebezzam! I'll review it this afternoon and let you know my comments.
@GJBoth and @diptodip thanks for your comments! I'll take a look.
One dummy question: is off-axis really the same as your kykx?
From what I understand kykx
is tilting the plane wave illumination (which can shift the diffraction pattern like in the top row of Fig 7 of the paper), while off-axis is interested in sampling the destination field off-axis (Fig 1 and bottom row of Fig 7 in the paper).
@GJBoth and @diptodip I think I've addressed your comments! It's now vectorized for y and x and without if-else conditions. I've left the original code commented because I wonder if it's easier for someone to read that than the current, very dense version?
Also I found a small bug in the previous version (I had an extra factor 2 because in the paper they talked about the field size being increased by 2x to linearize the convolution, but within your propagator function the field is already padded). So now the result looks more like the original paper.
Below is from original paper (maybe they apply a gamma correction?)
As mentioned above, I think off-axis may be different from your kykx for tilting the illumination? Can you confirm? If so, I think it could be interesting to add a shift_yx
optional argument for shifting the destination sampling window as done in the off-axis paper.
@ebezzam This all looks awesome! Sorry I'm just now getting to see this. By the way, I'm going to update this PR to merge into hackathon_2024
instead of main
. I'll leave some more specific comments but in general:
@GJBoth and @diptodip thanks for your comments! I'll take a look.
One dummy question: is off-axis really the same as your kykx?
From what I understand
kykx
is tilting the plane wave illumination (which can shift the diffraction pattern like in the top row of Fig 7 of the paper), while off-axis is interested in sampling the destination field off-axis (Fig 1 and bottom row of Fig 7 in the paper).
Yes I think you're right about that. And Rainer also made the great point today that we could simply add a new property to our Field
objects to keep track of a spatial location (destination sampling window center as in the paper you shared). This way when we propagate off axis we can just update this property instead of requiring propagation of a large field of view (of course both could still be allowed).
So this I think is definitely a nice thing that we should add, but perhaps we work on that update in another PR?
Below is from original paper (maybe they apply a gamma correction?)
It does look like a gamma correction. What does it look like if you plot your results with a gamma correction (e.g. intensity**0.7
or so)? Or they plot amplitude and not intensity (apologies, still haven't had time to take as close a read as I should)?
@diptodip thanks for your comments! I'll address them on Monday
@diptodip, @GJBoth I've addressed your comments, thanks for your support!
I've created a notebook which shows BLAS and off-axis propagation:
I've added a shift_yx
optional parameter to compute_asm_propagator
so that there is a working implementation of off-axis, but if you think it's better to move that to the Field
class, it should be straightforward.
Let me know if you need anything else!
Thanks Eric! We'll merge things into the hackathon_2024 branch and go from there. Really appreciate all your work implementing this!
Cool stuff you added! Looks great. Also the off-axis. Nice.
@GJBoth rough example and implementation to add support for BLAS (bandlimited angular spectrum)
References:
I've tried to create an example that replicates Fig. 9a of the original paper. It's not exactly the same (but close). We do see the high-frequency noise found in the original AS being suppressed.
TODO: