Basically a FresnelWavefront wrapper class that uses different unit convention #248
Comments
|
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. |
|
OK that's it for now. Very nice. Oh, and I see there is a conflict on Danke schön! |
|
I must note that this is my very first pull request ever. And I mean EVER. I am not very familiar with version control (nor am I a Python Pro). And yes, I am not familiar with writing unit tests. But I will have a look at the examples you mentioned and try to work it out. |
|
Well in that case congratulations! Learning enough of git and github to make a pull request is a significant accomplishment. :-) I wrote up some notes about how to write a test on an earlier pull request: |
|
I implemented some test cases and they seem to be fine. However, the CI test fails and I have no idea how to change that. |
|
Thanks! Yes I had seen your test cases commits come in earlier. Those look very good - if you hadn't told me before I would not guess this was your first time working with unit tests or pull requests! The CI system intentionally runs the same tests on a variety of versions of Python and the major libraries. The test is only failing on the setup with the very oldest versions. From the log messages at https://travis-ci.org/mperrin/poppy/jobs/376381937, it looks like something in the older setup is upset to encounter a Unicode degree symbol in the comment string on line 40 of physical_wavefront.py. I expect if you change (We could also try to diagnose more precisely why a Unicode character is incompatible with the older versions, but honestly it's just a comment string so I think it's easier to just avoid this rather than digging in any deeper.) |
|
Well, only took me a week to get it right :) |
Thursday May 03, 2018 at 08:10 GMT
Originally opened as mperrin/poppy#248
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.
DaPhil included the following code: https://github.com/mperrin/poppy/pull/248/commits
The text was updated successfully, but these errors were encountered: