Fresnel pull request for commenting

[mperrin]

Issue by mperrin
Friday Aug 21, 2015 at 17:36 GMT
Originally opened as mperrin/poppy#108

---

This is the whole fresnel branch including my recent changes. PR for commenting as requested by @josePhoenix in mperrin/poppy#103 (comment)

In retrospect I probably should have made a branch from the branch and pull requested from that, but I didn't think of it.

---

mperrin included the following code: https://github.com/mperrin/poppy/pull/108/commits

[mperrin]

Comment by douglase
Friday Aug 21, 2015 at 20:37 GMT

---

The notebook looks great!
But it is failing for me because the pyfftw import has been missing since eb09a36, I fixed this in #107.
I will test with pyfftw uninstalled for the moment.

[mperrin]

Comment by mperrin
Friday Aug 21, 2015 at 21:13 GMT

---

@douglase I just merged #107 for you so you can try it with pyfftw now.

Meanwhile I've managed to reproduce the error you're getting, although it's not totally clear to me why; on one computer it works fine in 2.7 but fails in 3.4, but on another computer I can get the ufunc casting error on 2.7. Investigating... but I'm not optimistic I will figure this out in the next 15 minutes before I have to split. :-)

[mperrin]

Comment by douglase
Friday Aug 21, 2015 at 21:13 GMT

---

side note on notebook: GaussianLens() is Gaussian because it's spherical, so it is a "Gaussian Optics" approximation of a lens... SphericalLens would be clearer.

[mperrin]

Comment by mperrin
Friday Aug 21, 2015 at 21:19 GMT

---

Or "QuadraticLens" maybe? The point is that it's a pure quadratic phase term, such that it converts a plane wave into a converging spherical wave or vice versa. In hardware this would be achieved by a parabolic mirror (e.g. off axis paraboloid). Using a real spherical lens or mirror doesn't get you a perfectly converging beam, it gets you a converging beam with spherical aberration. As no astronomer should ever forget. :-)

For that matter we could just name the class "Lens". I can imagine a future version of this which has a subclass "ConicLens" that allows precise description of conic asphere optics (such as the actual mirrors of Hubble, or the optics of JWST which are an ellipse, hyperbola, and ellipse, etc).

[mperrin]

Comment by douglase
Friday Aug 21, 2015 at 21:23 GMT

---

yes, spherical optics, not a spherical optic... I like QuadraticLens.

[mperrin]

Comment by mperrin
Wednesday Nov 04, 2015 at 09:52 GMT

---

I am planning to go ahead and merge this into master pretty soon. Any additional comments or issues before I do so?

I took a pass at cleaning up the fresnel.py codebase to be closer to PEP8 compliant; mostly minor formatting stuff. One thing I did not fix yet: the PEP8 inspector in PyCharm complains about the function names for FresnelWavefront.z_R and FresnelWavefront.R_c since those are not lowercase. This is an instance where the common mathematical notation for these variables is not in keeping with Python's style conventions. We could rename these to e.g. z_rayleigh and r_curvature, and maybe that would actually be clearer (though more verbose). But it's also nice to maintain a close consistency with the mathematical notation. Opinions?

[mperrin]

Comment by douglase
Saturday Nov 07, 2015 at 20:22 GMT

---

I don't see any issues with merging into master. I just created an issue for adding anti-aliased apertures (#129), since that seems to be the missing feature for head-to-head comparisons with PROPER, but that is not a just Fresnel issue. I have the PROPER example microscope working and see less than than 1% difference between POPPY and PROPER (https://github.com/douglase/poppy_example_notebooks/blob/master/Fresnel/Microscope_Example.ipynb). Have not had time for more examples (the TDEM coronagraph example is built but not well tested), but I don't think that is a reason to hold up merging with master.

I think the mathematical notation makes the code a bit more readable than the PEP8 versions, but understand the desire for consistency.

[mperrin]

Comment by mperrin
Monday Nov 09, 2015 at 16:39 GMT

---

Ok thanks much. Nice to see the microscope example! After further reflection I think we can keep the shorter names, I'll just make them all lowercase for PEP8 compliance. And I think it's a little easier to remember all lowercase z_r and r_c as opposed to mixed case (oppositely for the two parameters!). That sound fair enough?

[mperrin]

Comment by josePhoenix
Monday Nov 09, 2015 at 16:41 GMT

---

👍 to lowercasing. I also thought about providing short aliases to the longer names but it's probably not worth the additional confusion.

[mperrin]

Comment by josePhoenix
Monday Nov 16, 2015 at 15:31 GMT

---

Woohoo!