Proposed poppy.Instrument refactoring

[josePhoenix]

While trying to refactor the Jupyter Notebook GUI into a mission-agnostic set of functionality, I came to the conclusion that we need to clarify the responsibilities of the Instrument class. My first idea was to have an Instrument model and a Calculation model, but that has the two disadvantages of there a) not being enough Instrument-specific state that is not also calculation-specific to motivate splitting it up, considering b) that there is an additional cognitive overhead to a more complex API (and the inferior ergonomics of typing such things out when working interactively in a notebook or shell).

SO!

Instead, I propose refactoring the Instrument classes to encapsulate all of the necessary state for a calculation. This would include things that are already attributes on Instrument (filter, pupil, etc.), things that are in the options dict (jitter, jitter_sigma, etc.), and things that are arguments to calc_psf (fov_arcsec, fov_pixels, etc.).

Instead of providing a lot of optional arguments to calc_psf, I propose replacing it with methods for three types of calculations:

• Instrument.calc_psf(display=False) - Calculate a broadband PSF with the configured optics and output options
• Instrument.calc_psf_monochromatic(wavelength, display=False) - Calculate a monochromatic PSF
• Instrument.calc_psf_cube(wavelengths, display=False) - Calculate a len(wavelengths)-plane spectral cube for use in, e.g., Pandeia

I also have some ideas about using Python property objects to enforce consistent state in the Instrument models. Since this can cause some surprises for advanced users (e.g. auto_pupil in the past) we could have a single switch for all "automatic" changes (inst.validate_config = False?).

I've got a partial prototype in my POPPY repo. I hope to have it more fleshed out this week.

[mperrin]

Can we step back a level for a second? What is the fundamental problem that you are trying to solve here?

I think the bar should be pretty high for backwards incompatible API changes, which this would be. That's a lot of pain for end users and I'm not convinced that it's necessary.

[josePhoenix]

Fundamentally, there are three places to store the state that determines the output of a PSF calculation, when there should only be one. We've noticed this in the past with competing definitions of the oversampling factor, for example. This is also a problem for generalizing the GUI code, since a new model object is needed to hold calculation-related state and then instantiate and apply it to an Instrument instance at calculation time. (It seems to me that Instrument was originally created to hold calculation related state, then instantiate an OpticalSystem, apply said state, and perform a calculation-- so I feel that adding another wrapper layer is the wrong move going forward.)

Another option, that I think would not be as convenient for interactive use, is a Calculation object that does what I described.

It's definitely possible to make these changes incrementally (e.g. have an instrument.fov attribute that you can assign astropy.units.pixel or astropy.units.arcsec Quantities to, and still retain the instrument.calc_psf(... fov_arcsec= ...) keyword argument.

[josePhoenix]

This is less "this should go in the next point release!" and more "we need to improve the Instrument class internals, and I believe this would improve the API as well". For example, there is a default instrument FOV:

        if fov_arcsec is None and fov_pixels is None:  #pick decent defaults.
            if self.name =='MIRI': fov_arcsec=12.
            else: fov_arcsec=5.

There is an fov_pixels attribute, which takes precedence over fov_arcsec. And there's fov_arcsec which specifies an FOV in arcseconds, unless pre-empted by the options['parity'] value. All of these cases are different ways to specify one thing, which is the FOV of the requested region in the calculation.

A better API could be:

from astropy import units as u
inst = poppy.Instrument()
inst.fov = (15 * u.pixel, 10 * u.pixel)
psf = inst.calc_psf()

Or

from astropy import units as u
inst = poppy.Instrument()
inst.fov = 2.0 * u.arcsec

Warnings could be emitted in the cases where the FOV exceeds the nyquist sampling of the pupil, or where rounding is required to get an integer number of detector pixels across the FOV.

Or to explicitly request a particular parity:

from astropy import units as u
inst = poppy.Instrument()
inst.set_fov_arcsec(2.0, parity='odd')

[josePhoenix]

This has the additional benefit of making it easy to see what FOV you're going to get if you don't specify one, without looking in the code. (Simply print(inst.fov)!) Using @property with setters also allows the class to enforce a consistent state at all times. (I believe this would also help with the problems we saw with inst.display() and deferring some validation to calculation time.)

[mperrin]

There's another conceptual distinction though, which is part of the original motivation for why different parts of calculation state are treated differently. We are of course trying to model the behavior of actual physical objects which can be configured in various ways. To keep the API at least somewhat intuitive for users, the Instrument class is supposed to at some level map onto the actual physical properties of the instruments (detector choice, filter wheel settings, selection of optional stuff like coronagraph or spectrograph masks, etc). Then on the other hand there are aspects that are truly calculation specific, like oversampling or size of output FOV. The original intent was that the Instrument properties handle the former, and function arguments to calc_psf handle the latter. I agree it's gotten a little less clear cut over time, and that there may be better ways to do things. But I'm not convinced that making everything a property is the right approach.

[josePhoenix]

The problem is that we have multiple keyword arguments to calc_psf mapping onto the same thing. Additionally, users of the API need to look in at least three places to find all the options available to them instead of doing something like instr.<tab> in their shell or looking at the docs page for the instrument of their choice.

I see the argument that Instrument should represent mechanisms and calc_psf should represent calculation-specific options, but I'm not sure the current arrangement is actually making it easier for users to discover the functionality available. I started thinking about this in the context of building a GUI, in which case we have no one model for the state of a calculation (of the sort a user may perform in the old or notebook GUI). I can build such a model class, but I would rather see if I can make the underlying model more consistent and easier to reason about.

[josePhoenix]

Another API could be mechanisms as attributes of instruments, and calculation options as attributes of a PSFCalculation object initialized as an attribute of the instrument:

inst.filter = 'F1000W'
inst.psf.fov = 10 * u.arcsec
inst.psf.source = pysynphot.Blackbody(5700)
inst.calc_psf()  # as an alias to inst.psf.calc() 

[josePhoenix]

I solicited some comments from an anonymous WebbPSF user 😉

User: So, hm. I agree that it's confusing to be able to have conflicting attributes and arguments passed to calcPSF, but I also agree with Marshall that it makes conceptual sense to keep a distinction between instrument properties and calculation parameters. But in practice that distinction doesn't work out… I like the idea of the PSFCalculation object, but I also wonder if it just adds conceptual complexity without much gain in usability.
User: Maybe I'd need to see it used more in practice. In your example, you're still considering the filter a property of the Instrument, but that's something that I could see a user wanting to switch per PSFCalculation, so it seems like you'd end up in a similar situation to before, except now you're switching attributes on two objects for a calculation instead of switching attributes on an object and arguments to a method.
Joseph Long: Well, the PSFCalculation validates the various attributes of the calculation and still reads instrument mechanism configuration from the Instrument instance
User: I guess my point was that, from the perspective of the user, instrument mechanisms and calculation attributes are both just parameters to be modified for the PSF calculation, and so they're still going to multiple places to set parameters for each calculation. But maybe it's still cleaner.
Joseph Long: Well, that's sort of my rationale for making everything an Instrument attribute
User: Yeah. I don't hate that idea. It's maybe conceptually confusing if you're expecting a correspondence between physical and virtual objects, but not if you're just treating the Instrument as a tool for producing a PSF.