Add enhancement #226 - Githubissues

corcoted commented 7 years ago

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 overlap

coveralls commented 7 years ago

Coverage decreased (-0.1%) to 65.315% when pulling 1d5becff9fde094f8d5a022c87702ecf4b5a0988 on corcoted:add-226 into 6b01e66b685cf2c82e89805c28ac502287a034fb on mperrin:master.

mperrin commented 7 years ago

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:

Could you please add a unit test case that tests the proper functioning of the new option? This is doubly helpful since it would also be a small bit of example code showing the use of the new option.
- What about the case of apertures with partial transmission (gray pixel values between 0 and 1)? What do you think the desired behavior is or should be for such cases? This might not often be a relevant case for most users, but it would be nice to have the documentation clearly explain how the "and" and "or" options work including for edge cases like this. Similarly right now the doc string doesn't say anything about the correction for overlap. (Before I read through the code, I was concerned that adding transmissions would end up with total transmission greater than 1, unphysically, so a quick explanation that it doesn't do that would be appreciated in the docs string).

Thanks again - I really appreciate both your suggesting a new feature and immediately providing a PR implementing it. :-)

mperrin commented 7 years ago

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.

corcoted commented 7 years ago

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.

mperrin commented 7 years ago

The other way to ensure the transmission stays between 0 and 1 is just to enforce that constraint directly: trans = np.clip(trans, 0, 1) will truncate values above 1 to be equal to 1. Not sure which is preferable; I'm not sure either is particularly more physically motivated once you start adding non-overlapping transmission regions in this manner.

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 tests/test_optics.py at test_CompoundAnalyticOptic for instance. Here's how:

write some function that makes an OpticalSystem or an OpticalElement instance or whatever, and has appropriate function calls to use the feature you want to test
add one or more assert statements that checks the truth value of something that should be true if your feature is working correctly. For instance you could check the array values in the combined transmission are 1s and 0s at particular pixel indices corresponding to the overlapping or non-overlapping regions.
make sure that your test function is named something starting with test_ and is inside one of the files in the tests subdirectory. That will get it automatically found and executed by the tests system.

That's it! Let me know if you have any more questions about that.

corcoted commented 7 years ago

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.

coveralls commented 7 years ago

Coverage decreased (-0.1%) to 65.315% when pulling 1b073a2561e928896c754b6fca6f5602bcc8cbd9 on corcoted:add-226 into 6b01e66b685cf2c82e89805c28ac502287a034fb on mperrin:master.

corcoted commented 7 years ago

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.

corcoted commented 7 years ago

Fixed! I needed to define get_phasor() for the new mode. For my own future reference, AnalyticOptic subclasses need to define get_transmission(), get_opd(), and get_phasor().

@mperrin I think it's ready to go.

mperrin commented 7 years ago

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!

corcoted commented 7 years ago

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 Sent: Sunday, July 9, 2017 4:37 PM To: mperrin/poppy Cc: corcoted; Author Subject: Re: [mperrin/poppy] Add enhancement #226 (#227)

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! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

This email has been checked for viruses by AVG. http://www.avg.com

mperrin commented 7 years ago

Great! I will test this locally myself later today and then merge. Much appreciated.

mperrin commented 7 years ago

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!

corcoted commented 7 years ago

Yes, that’s right. Thanks!

Sent from Mail for Windows 10

From: Marshall Perrin Sent: Monday, July 10, 2017 8:29 PM To: mperrin/poppy Cc: corcoted; Mention Subject: Re: [mperrin/poppy] Add enhancement #226 (#227)

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! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

This email has been checked for viruses by AVG. http://www.avg.com

mperrin / poppy

Add enhancement #226 #227