mperrin
commented
6 years ago Thanks! This looks very good. I am definitely happy to have this added in.
As you've probably inferred this is a very different use case from our needs here that motivated me to start poppy (thermal blooming from high energy lasers is not a relevant concern when observing ultra-faint galaxies with a space telescope! :-) ) but I am very happy to have this package grow to address other use cases in physics too. It's very nice to have someone contributing expertise in an area of physics I'm less familiar with - thanks.
I'll take a look through the code and offer any suggestions that come to mind. The main thing I would want to see before merging this is some unit tests that verify the basic functionality works as desired. Probably can be adapted from the example cases in your demo notebook. But you'll be the best person to write those up. Doesn't need to be 100% coverage (we don't have anywhere near that already), but I'd like at least one or two basic tests for the PhysicalFresnelWavefront class. If you're not familiar with writing tests for py.test let me know and I can assist, but the files in the tests subdirectory should I hope be clear enough examples.
Units is a messy topic. There's about a dozen different frameworks for handling them in Python (see list at https://pint.readthedocs.io/en/latest/faq.html). We use the astropy one since that's the most common among astronomers, but I realize it's a new thing to learn for non-astronomers so can be a barrier to more general use. I don't mind your not using them in all possible places. Personally I like having units on the parameters rather than having to remember what units are on a bare float, but already the code is hit-or-miss on that for historical reasons rather than entirely consistent.
DaPhil
coveralls
Based on this issue I wrote some time ago I wrote a wrapper class for the FresnelWavefront class to use physical units for the wavefront (i.e. V/m) and intensity (i.e. W/m^2). I also added some additional capabilities like computing the beam radius, center, and quality factor based on 2nd moments (i.e. ISO norm conform). If the pull request is accepted, I can provide an OpticalElement class that represents a thermal blooming phase screen (for which physical units of the intensity is necessary since it is a non-linear effect). In its current state, the class works but is probably not conform to how the main authors want a class to work in poppy. For example, I did not make use of the astropy.units package whenever I could. My main intention is rather to get an impression if this class and a thermal blooming phase screen is interesting enough to be added to poppy. If so, I am happy to work on the class to conform to any programming guidelines, naming conventions etc.
Unrelated note: I have also expertise in Kolmogorov phase screens and have noticed that there is a branch dedicated to that where I can contribute if required.