-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
Corrected labeling between ASM/Fresnel #122
Conversation
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.
Please see my comment on odak.wave.transfer_function_fresnel
from my revision. I suggest also updating your pull request but this time, could you also please update THANKS.txt
and CITATIONS.cff
. Such that your name is included. Thanks.
odak/wave/classical.py
Outdated
U1 = np.fft.fft2(np.fft.fftshift(field)) | ||
U2 = mask*U1 |
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 observe that with the new additions, this type of beam propagation generates a different result than before:
I generated the above result by running test/test_wave_propagate_beam.py
for distance of 5e-3
. On the other hand, the Bandlimited Angular Spectrum
generates the following result when added to the list of propagation_types
in that unit test:
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.
Thanks, I included the name now.
I'm not sure if I'm merging correctly. Now I removed the minus sign in propagation direction and test locally for 5e-3 m prop distance in test_wave_progate_beam.py. They look the same for transfer function kernel, impulse response kernel, bandlimited AS kernel and AS kernel. Please test again and let me know if it still not works for you, or if you prefer me to create a new pull request.
Transfer function kernel in wave.classical
Impulse response kernel in wave.classical
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 can't reproduce the above results on my end. Could you please make sure to completely remove odak
and install the version that you have updated from your repository?
If you are using pip
for installation, please first make sure that you have removed it from your local by typing pip3 uninstall odak
. In some cases, people use sudo
in their installation. To make double sure that you have completely, removed it. Please make sure to type sudo pip3 uninstall odak
.
To install it using your local version, navigate to the directory that you have your odak
using a terminal (e.g., cd PATH/odak
). Once you are at the root directory, simply type pip3 install -e .
to install your version.
Please also revert the sign modification from odak.learn.wave
as this would break many of our existing repositories.
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 confident that you are reproducing the above results using
odak
inGithub:kaanaksit/odak
as I can reproduce the same result as yours when I use that.
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.
Ah I see. Yes you are correct. I was using previous odak
by mistake. Now I can see my version producing the error and they have been fixed now.
I also revert the sign modification in propagation direction. Please test and let me know. Thanks.
…sical and learn.wave.classical
- the Band-limited AS should directly be multiplied by the window if H is defined by exp or use (amp * cos + 1j * amp * sin) if defined without exp. - fixed the previously missing fftshift in H - revert the modification on propagation direction sign convention
-corrected the label between ASM, Impulse Response and Transfer Function Fresnel.
-removed mask in vanilla Transfer Function Fresnel because it's not band-limited. Now it also matches with learn.wave.transfer function kernel
-re-define the freq sampling in vanilla Transfer Function Fresnel
-removed z in Impulse Response Fresnel because it might confuse with distance.
-removed extra pair of fftshift in Band-Limited Angular Spectrum to match the kernel and field
-removed np.roll in the end of Angular Spectrum