mperrin / webbpsf

James Webb Space Telescope PSF simulation tool - NOTE THIS VERSION OF REPO IS SUPERCEDED BY spacetelescope/webbpsf
BSD 3-Clause "New" or "Revised" License
16 stars 15 forks source link

Move 'CLEAR' from pupil_mask_list to filter_list in NIRISS #64

Closed josePhoenix closed 8 years ago

josePhoenix commented 9 years ago

Based on correspondence with André Martel, it seems like NIRISS users will find our API less astonishing if 'CLEAR' were a filter option, as it is in the physical instrument, rather than a pupil mask option. This will also make it straightforward to compare with ground test data taken in filter = 'CLEAR' + pupil_mask = 'CLEARP' mode.

This may require some special casing, since 'CLEAR' is not, in fact, a filter. My current plan is to use a top hat transmission curve across the wavelength range of NIRISS (0.6 - 5.0 um).

mperrin commented 9 years ago

Does André also want us to make the filters that are physically in the pupil wheel into pupil options?

We should discuss this before jumping right into implementation. Thus far the development paradigm for webbpsf has been to gloss over some specifics of instrument implementation in favor of a slightly more platonic ideal conception of each. For instance for NIRCam we ignore the fact that some filters are in the pupil wheel, and likewise for MIRI that there are pupil masks co-mounted with the filters in the filter wheel.

So again keeping with the principle of least astonishment, if we're going to revamp one class to mimic how the widgets are actually stuck inside the box, we should consider doing so for all of them.

josePhoenix commented 9 years ago

Ah, I see what you mean. The motivation here is that they want to simulate certain configurations that would map to setting filter = None and having two settings for the pupil wheel in WebbPSF-land. There's currently no way to do that within the API exposed on the NIRISS class.

I sent André a workaround that uses the manual source spectrum specification, but if the combination of CLEAR + GR700XD and/or CLEAR + CLEARP end up being useful to the instrument team, it should be possible to specify them somehow.

I agree that rearranging all the instrument classes may cause a bit too much breakage. Let's discuss this with NIRISS via email.

mperrin commented 9 years ago

I think that defining 'CLEAR' as a filter using a box function as you describe is actually totally legitimate.

Now that I've thought about this some more I may have been overcomplicating this by jumping right to the question of "do they want us to make the class API look like the instrument physical mechanisms?". If all that's needed is adding a clear filter then I think that's comparatively straightforward.

mperrin commented 8 years ago

I think this issue can be closed - the CLEAR filter you added back in April is sufficient, and I don't see any need for any additional change on this.

josePhoenix commented 8 years ago

I think I was leaving it open as a reminder that it's not actually fixed until the new data package is out. But that'll be tomorrow, so yeah. Closed is good.