Add enhancement #226 #227
Comments
|
Thanks! Sure, this is a nice option to add and I can see why you want it. And the implementation looks good. A couple quick questions:
Thanks again - I really appreciate both your suggesting a new feature and immediately providing a PR implementing it. :-) |
|
And Just to add, it looks like the Travis tests are OK; the one test job that failed is the astropy development version, and we already know that dev astropy is causing test failures for an unrelated reason. |
|
Great! Thanks for the feedback. I’ll definitely add some documentation about how the “or” setting works. I don’t have a strong argument for the method I chose in regards to cases with fractional transmission. My chief concern was to keep the resulting transmission between 0 and 1. This seemed like a good way to do that: in effect, I’m multiplying the opacities: (1-Tout) = (1-T1)(1-T2) => Tout = T1+T2-T1*T2. If anyone has another suggestion, I’m happy to discuss this. One other concern I had was about how to handle the OPD. In the present code the OPD is simply added, but there could be unexpected results in regions where one of the constituent optics has places where the OPD is nonzero but the transmission is zero. Any thoughts? I haven’t built unit tests before, but I’ll look at the existing code and see if I can figure it out. I might have questions later. |
|
The other way to ensure the transmission stays between 0 and 1 is just to enforce that constraint directly: As for OPDs, I'd say that the case you describe (a nonzero OPD but zero transmission at the same location) isn't physically meaningful - how would that even make sense in a physical optical system? I'm not particularly concerned if the software behavior isn't well-defined for handling a nonphysical case. Writing unit test cases is straightforward using the py.test framework. Take a look in
That's it! Let me know if you have any more questions about that. |
|
I did consider clipping the transmission. Another option is to add the transmissions, and if there are any regions with T>1, then divide by the maximum T. This last option was my first attempt, based on my own use case. Any preference? Regarding OPD: sure, it's physically meaningless to talk about OPD when the transmission is zero, but it can be convenient to define the OPD everywhere, even in T=0 regions, if you're trying to apply phase structure to the whole aperture. This is lazy programming (should set OPD=0 when T=0), but I did this exact thing (see below) and wasted time trying to debug it. (In retrospect, I would have been better off defining the complex phasors and then extracting the transmission and opd from that.) Ok. The tests look straight forward. I'll see what I can do. Background info: (TL;DR: I'm trying to model compound light sources.) The reason I started working on this is that I want to model a system where the light source is a fiber array outputting monochromatic light. I have an array of optical fibers, which I'm approximating as circular apertures for now, and each fiber is tilted by a different (small) angle. I model the tilts by applying an OPD gradient across each aperture (this step caused me grief when debugging). Then I combine these apertures into one "optic." From there I use the Fresnel propagation routines to look at the combined diffraction and interference effects, including downstream lenses. In the end I wrote a custom AnalyticOptic class to build the fiber array, but I was looking for a more efficient to do similar things than defining the transmission and opd for each coordinate using a bunch of if statements to choose the right fiber for each coordinate. CompoundAnalyticOptic almost does what I need, but not quite. |
|
I found a problem while testing and need some advice. My CompoundAnalyticOptic with the new 'or' combination method gives the expected transmission for the OpticalElement, but when I try to include this object in an OpticalSystem and do calcPSF, I get unexpected results. I'm still trying to find the problem exactly, but the amplitude array of the initial Wavefront is incorrect. Either the scale is changing in a way I don't want, or I'm still getting the original CompoundAnalyticOptic behavior (what I call 'and') for these calculations. |
|
Oh that's interesting. You're not supposed to have to redefine get_phasor for each subclass, it should be enough to just define get_opd or get_transmission, and then the parent superclass' version of get_phasor gets called. But CompoundAnalyticOptic is a bit of a special case. It actually looks to me like it has a redundant code path for get_phasor which is left over from an earlier version of poppy before we had split out get_opd and get_transmission as separate functions. Looks like this may be a bug that's my fault that you just discovered. Excellent, thanks! I predict you could instead delete entirely the implementation of get_phasor from your CompoundAnalyticOptic and everything would still work correctly then; it'll just call the superclass AnalyticOptic get_phasor which will in turn call the CompoundAnalyticOptic get_opd and get_transmission. Good sleuthing to figure out the problem was with get_phasor. Thanks! |
|
Thanks for the pointer! Deleting get_phasor from CompoundAnalyticOptic did work, so I’ve pushed that change. I think everything is ready to go now, unless you can think of another edge case that should be checked. From: Marshall Perrin Oh that's interesting. You're not supposed to have to redefine get_phasor for each subclass, it should be enough to just define get_opd or get_transmission, and then the parent superclass' version of get_phasor gets called. But CompoundAnalyticOptic is a bit of a special case. It actually looks to me like it has a redundant code path for get_phasor which is left over from an earlier version of poppy before we had split out get_opd and get_transmission as separate functions. Looks like this may be a bug that's my fault that you just discovered. Excellent, thanks! I predict you could instead delete entirely the implementation of get_phasor from your CompoundAnalyticOptic and everything would still work correctly then; it'll just call the superclass AnalyticOptic get_phasor which will in turn call the CompoundAnalyticOptic get_opd and get_transmission. This email has been checked for viruses by AVG. |
|
Great! I will test this locally myself later today and then merge. Much appreciated. |
|
Yes, that’s right. Thanks! Sent from Mail for Windows 10 From: Marshall Perrin BTW, @corcoted am I correct you are Ted Corcovilos? I'd like to add your name to the list of contributors to this project now, so I want to double check and make sure I've got it right! This email has been checked for viruses by AVG. |
Saturday Jul 01, 2017 at 18:22 GMT
Originally opened as mperrin/poppy#227
Patch for enhancement request #226
Added an optional parameter "mergemode" to CompoundAnalyticOptic which provides two ways to combine AnalyticOptics:
mergemode="and"is the previous behavior (and new default)mergemode="or"adds the transmissions of the optics, correcting for any overlapcorcoted included the following code: https://github.com/mperrin/poppy/pull/227/commits
The text was updated successfully, but these errors were encountered: