-
Notifications
You must be signed in to change notification settings - Fork 41
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
Bug in QuadraticPhase? #252
Comments
Ok, I just saw that the FresnelWavefront class overwrites the imul method to use the apply_lens method if a QuadraticLens is about to be applied. This method alters the beam parameters in the way that it delivers the unexpected values. But this leads to the situation that the curvature I want to apply at the aperture plane is not the one I defined. I just want to apply a quadratic phase to a wavefront that has a flat phase. In the meantime I checked the notebook for the Fresnel propagation. There, Example 2: Divergence of a Gaussian Beam near the Beam Waist and Adding Lenses fail when other units then meters are used. |
Ok, QuadPhase is the thing I need to use to overcome this issue. I don't think that this is the intended behavior (since it is used in the notebook incorrectly)? |
The units issue definitely sounds like a bug. As for applying the phase you want, you’re right there’s a subtle distinction between just applying a phase to a wavefront while leaving it “flat” in terms of the beam curvature, versus applying a phase that changes the curvature. I believe you can have the effect you want by using the ThinLens class rather than QuadraticLens. But this is a little non-obvious and I wonder if there’s a better way to do this. @douglase what are your thoughts on this? |
Ah yes, you can use QuadPhase rather than QuadLens. We could clarify the distinction better in the docs. Sounds like that notebook may need an update too |
Examining the log clarifies a bit what is going on. Here's examples of the output log for the two cases of using QuadPhase and QuadraticLens. (Note, I added some additional log print statements equivalent to your 'print(self.z)' so there's some extra output in the following compared to the code on GitHub) Using QuadPhase: wf = poppy.FresnelWavefront(beam_radius=1.0*u.m,
wavelength=1.0*u.m,
units=u.m,
npix=128,
oversample=1)
wf *= poppy.QuadPhase(z=-100*u.cm, name='QuadPhase')
Using QuadraticLens: wf = poppy.FresnelWavefront(beam_radius=1.0*u.m,
wavelength=1.0*u.m,
units=u.m,
npix=128,
oversample=1)
wf *= poppy.QuadraticLens(f_lens=-100*u.cm, name='QuadraticLens')
|
The notebook example is a little over complicated, since it includes two lenses, but it illustrates how you add lenses in a way that updates the pilot gaussian beam parameters and seems to run OK. Without those updates, the propagation algorithm will break down not knowing the distance to propagate to the next beam waist. It might help to add a cartoon ray trace of the system. It's confusing that z has multiple meanings (see https://github.com/mperrin/poppy/blob/master/poppy/fresnel.py#L54 where it was suggested it be renamed focal length). I should dig up the reference to blame it on. But in that context, the example at the top is correct, z=-10. It makes sense that the applied radius of curvature in Marshall's example is not exactly 1m since the beam has some divergence without the lens. But, I am confused the difference between the debug statements in Marshall's example and the quoted number in the first comment, 9.869604401089358 m vs 986960.4401010844 m. they are very close to 1e5 off from each other. @DaPhil, can you confirm the units? @mperrin can you push your more descriptive debug version? I always like having more debug statements. |
The notebook fails e.g. in Example 2: Divergence of a Gaussian Beam near the Beam Waist when using
There is a typo in my very first statement. it yields +10 m when I define
The ratio is due to different values used by me and @mperrin: 1mu vs 1m for the wavelength and 10m vs 1m for the curvature, so the get 1e6 from the wavelength and lose 1e1 from the curvature. I encountered this issue in the following code (originally comparing poppy to proper):
You will see that at 1900 m, poppy produces antialiasing (while proper does not). |
From the
The wavelength is never scaled to meters. But it should? Or should one always use the variabel _wavelength_m instead? |
yikes, it does look like something is up with that aliasing. I can reproduce it by propagating directly to 1900*u.m with this minimum working example taken from @DaPhil's post:
|
@DaPhil on closer inspection, I'm not as sure this is a bug. I haven't been able to find any errors in the calculation and if I increase the oversampling to greater than unit, for example 10 in the image below, things look reasonable: Would you mind sharing you PROPER example you are comparing to? Particularly the beam_ratio, since that is 1/oversample. |
(With respect to the note above about converting the units to meters, if the value has units of length, whether they are centimeters or parsecs, they will be converted to meters. If the user didn't specify the units, it's assumed they are meters (this is primarily to maintain backwards compatibility with earlier versions of POPPY which assumed meters everywhere). Do you see somewhere the code is not functioning this way? |
Bug is certainly not the right word for it I think. I also noticed that I can make it work by increasing the oversampling and resolution. But first, this heavily increases the computational cost (and at the moment I only can use my 6 years old MacBook Pro for development). And second, proper delivers a different result with the same starting conditions.
Propers beam ratio is something different than poppy's oversample. The first determines the length of the grid size. I think this is what poppy's beam_radius does. In addition, the oversample fills zeros beyond that beam_radius variable. Did I understand the code correctly when I state this? Anyway, here are the files with the code that compares poppy and proper with an identical initial wavefront:
Have you try to explicitly use units in your Fresnel notebook, example 2, e.g. replacing 1e-6 with 1*u.micron for the wavelength? It doesn't work. I think the wavelength is never scaled to m properly. In poppy_core.py, look at lines 127+:
I think there should be a scaling of the wavelength property itself if it is an astropy.quantity, e.g. it should be
With this change, your notebook runs fine even when units are used. |
aliasing:have you tried propagating the same array in both libraries? POPPY doesn't have smoothed apertures yet (see #129 and #180) so the circular function is slightly different. unitsthe comparison in the notebook didn't do any unit conversion, so it isn't printing the right values. If I don't change poppy_core but add units and replace all the .values with their decomposed units the "Example 2: Divergence of a Gaussian Beam near the Beam Waist" it works:
your suggestion is still a good one, since it simplifies the mental arithmetic if someone sees a length that is inches^2/millimeter. But maybe we should make the default unit configurable? @mperrin thoughts? |
Aliasing: Agreed re: doing tests with the same sampling. Note that the minimum recommended value of sampling is 2. (i.e. 2 points per lambda/D, so it's Nyquist sampled). Anything less than that is going to have potentially surprising results. Right now poppy issues a warning for sampling lower than that, in
It turns out these are two slightly different ways of specifying the same things:
In other words these should be equivalent: wf = poppy.FresnelWavefront(beam_radius=0.5*u.m, npix=1024, oversample=2,
wavelength=1*u.micron)
Both of those should produce wavefront arrays which are 2048 total pixels across, but have the beam in only the middle 1024 pixels. The outer pixels are all zeros. The inner 1024 pixels will represent 1 meter in both cases, specified by diameter in PROPER and by radius in poppy. Note that poppy's default (Aside on antialiased apertures: For circular apertures, the sub pixel geometry calculations for gray pixels are actually working well in the Units: In general, the units framework is supposed to allow you do to calculations and get correct results without having to worry about unit conversions explicitly. It will all happen behind the scenes such that things are computed with any necessary conversions applied. That said, for display and output purposes you're right it makes sense to either cast to meters or decompose to base units (which do precisely the same thing, for lengths), just so you don't see displayed values in nonstandard composite units like inch^2/mm or mm^2/meter or so on. I don't really think it's worth the effort to put in a configurable default unit. I don't see a driving need for being able to toggle arbitrarily - if everything's shown in meters, it's pretty easy to scale in your head when other units are needed. :-) |
Hi,
I just discovered something I am pretty sure is a bug. Although I tried to dig deep in the source code, I just cannot see where it is coming from. Try the following:
and inspect the value of the radius of curvature. I did this by printing to the console the property z in the QuadPhase get_phasor function:
In the above code, I get printed out -10 m (instead of a positive value). Now change the units for the wavelength to something different, e.g. mm or so when initializing the wavefront. The output reads 986960.4401010844 m.
I tried to look at what is happening but I cannot figure it out.
The text was updated successfully, but these errors were encountered: