Closed mperrin closed 6 years ago
Comment by josePhoenix Tuesday Aug 18, 2015 at 19:12 GMT
Hmm, upon reflection, I think I could change a few things to make them more consistent with the API for JWST instruments. (Mainly using the .pupil_mask
attribute for selecting whether a mask should be applied.) I could also use the .detector
and .detector_list
attributes that are created but not currently used on the JWST side, instead of adding .aperture_list
. (I'm not sure why I didn't do that in the first place!)
UX-wise, should I select a "default" field position and detector? Or maybe we should come up with some mean values for Z1-22 across the FoV and make that the default?
Comment by mperrin Tuesday Aug 18, 2015 at 19:25 GMT
I vote for default position, probably center of detector 1. That's relatively close to the middle of the whole FOV but unlike the actual center is not in between two detectors and thus unobservable. Also "center of detector 1" is an aesthetically pleasing default.
Comment by mperrin Tuesday Aug 18, 2015 at 19:30 GMT
Also, you probably didn't think to use the aperture_list
because it's not really used yet for JWST, but rather is a placeholder awaiting the addition of useful functionality.
Furthermore, the word aperture
is highly overloaded in this context, and its use as a technical term in the HST software ecosystem to mean "detector location with associated pointing geometry and potentially subarray" is not obvious to anyone who isn't steeped in HST/JWST technical minutia. Even in JWST land I have recently gotten push back from some people (e.g. the "PPS special requirement to override APERTURE when pointing the telescope" is going to be named "SIAF Fiducial Point Override" because the PPS team balked at the ambiguity of using the word "aperture".) I think detector
and detector_list
is potentially fine for WFIRST, to minimize unnecessary propagation of jargon.
That said, I agree with trying to keep the APIs consistent. Just that particular part of the API is not necessarily optimal.
Comment by josePhoenix Tuesday Aug 18, 2015 at 19:38 GMT
Currently, the detector (.aperture
) is always set for an instantiated WFI, but options['pixel_position']
is not. So, the second question is: Should an absent pixel_position
default to (2048.0, 2048.0)? If we're always incorporating field-dependent aberrations, maybe we should move it to an attribute on the instrument? The distinction between what goes in options
and what's an attribute is a little fuzzy to me.
Comment by mperrin Tuesday Aug 18, 2015 at 19:47 GMT
It's a fuzzy distinction, and was never thought out in detail.
It seemed useful to have a dict that one could just toss arbitrary metadata and annotations into, which might not apply to all instrument classes. In other words it's a catch-all, and therefore somewhat of a kitchen sink. But anything that is common to many instruments has a good case for being promoted to an attribute.
Comment by josePhoenix Tuesday Aug 18, 2015 at 21:51 GMT
Okay, 2d12b49 has a bunch of changes to make the naming consistent with the detector stuff on the JWST side.
options['pixel_position']
is now the detector_position
attribute, the detector stuff is bumped down a level from SpaceTelescopeInstrument
into two implementations in WFIRSTInstrument
and JWInstrument
so their different ideas about how to look up detectors don't conflict.get_aberrations
is now _get_aberrations
DETXPIXL
, DETYPIXL
and DETECTOR
header keywordsComment by josePhoenix Tuesday Aug 18, 2015 at 22:00 GMT
Possible to-dos:
WFIRSTInstrument.detector_position
pupil_mask
to specify a masked pupil instead of inferring from the wavelengths (I started doing this but it didn't seem more useful than what's already there, but maybe I should?)FieldDependentAberration.set_field_position
, as the data from Goddard don't go all the way to pixels (0, 0) and (4096, 4096) (presumably because of the reference columns)Comment by mperrin Tuesday Aug 18, 2015 at 22:11 GMT
For pupil_mask
you should just borrow the code around NIRISS CLEARP from webbpsf_core
. It is an exactly analogous situation. Having it be a pupil_mask
attribute that is set automatically by default but can be overridden if the user so desires is a reasonable behavior here.
Comment by mperrin Tuesday Aug 18, 2015 at 22:13 GMT
Also this is still lethal on Travis, in case you hadn't noticed that.
Comment by josePhoenix Tuesday Aug 18, 2015 at 22:16 GMT
Hmm, I think it'll mostly build now that the POPPY change is merged. (The two jobs that install stable POPPY will still fail though.) Just restarted https://travis-ci.org/mperrin/webbpsf/builds/76188969
Comment by josePhoenix Tuesday Aug 18, 2015 at 22:40 GMT
I just realized that Instrument.display
doesn't work with the whole "set pupil shape at calculation time" idea. I guess I could factor out the test code into its own function, and perform the pupil mask switch both when calculating a PSF and when assigning to Instrument.filter
Comment by mperrin Tuesday Aug 18, 2015 at 22:50 GMT
Shoot, you're right. And I just tested it with NIRISS and that also doesn't display the auto-set CLEARP pupil if you just display the instrument.
Would it fix this sufficiently to just add a call to _validateConfig
at the start of poppy.instrument.display()
?
Comment by josePhoenix Tuesday Aug 18, 2015 at 22:51 GMT
Nope, because the logic in validateconfig now wants to peek at the wavelengths requested (fixing the NIRCam pixel scale autoselection issue where it was only looking at filters and not selecting appropriately for monochromatic calculations, if I'm not mistaken)
Comment by josePhoenix Tuesday Aug 18, 2015 at 22:52 GMT
Comment by mperrin Tuesday Aug 18, 2015 at 22:59 GMT
More generally, I just realized that if one has wavelength-dependent aberrations (such as is the case, barely, with WFI) then to display
the optical system you have to pick a wavelength. So the display
infrastructure needs to guess some reasonable default wavelength, for instance central wavelength of the filter would be reasonable.
Comment by josePhoenix Tuesday Oct 27, 2015 at 15:16 GMT
The fix to the auto_pupil stuff depends on https://github.com/mperrin/poppy/pull/118
Comment by josePhoenix Monday Nov 02, 2015 at 23:22 GMT
It was requested that WebbPSF logging "not show up in red" in the IPython notebook. Changing that turned out to be trickier than expected.
Really, anyone using webbpsf.setup_logging
probably doesn't care that it'll clobber their root handlers.
By default, logging
has a StreamHandler with stream=sys.stdout
plugged into the root logger. To handle DEBUG/INFO
and WARN/ERROR/CRITICAL
output on different streams (required to get a less alarming color in the Notebook), we have to remove that StreamHandler and add two new ones: one that goes to sys.stdout
for DEBUG
and INFO
(using FilterLevelRange
) and one for sys.stderr
for the rest.
I also was getting an ImportError
about jwxml
so I added a .
.
Comment by josePhoenix Monday Nov 02, 2015 at 23:24 GMT
Oh, the down side is that restart_logging
runs on import whether you want it or not. It could be very frustrating to have an import side effect that messes with the logging configuration, considering how frustratingly opaque that whole library is to begin with.
I might remove the 'auto-start logging' functionality, convenient as it is. That way we don't touch anyone's logging setup unless we're asked with setup_logging
.
Comment by josePhoenix Monday Nov 02, 2015 at 23:58 GMT
Okay, I've verified that the logging config items aren't actually being saved any more in AstroPy 1.0+. The docstring has been updated to tell users to edit the config file by hand. There's an autoconfigure_logging
setting allowing users to opt-in to having webbpsf handle the logging config for them.
Comment by mperrin Wednesday Nov 04, 2015 at 09:55 GMT
I presume you have seen that the Travis build is still failing on this, apparently due to not being able to find valid coefficients for the model and trying to iterate over None
instead...
Comment by josePhoenix Wednesday Nov 04, 2015 at 13:51 GMT
That test failure looks very much like something I thought I had fixed. It might be kicking around in a local branch. I'll check when I get to work.
Comment by josePhoenix Wednesday Nov 04, 2015 at 20:08 GMT
@mperrin the only jobs that failed used stable POPPY, so everything important passed
Comment by mperrin Wednesday Nov 04, 2015 at 20:25 GMT
Does this need any changes related to the 2.37 instead of 2.40 meter pupil, or is that solely in the input pupil files? If there's no changes there then I think we are ready to go ahead and merge this?
Comment by josePhoenix Wednesday Nov 04, 2015 at 20:26 GMT
Those have been changed in /itar/jwst/tel/share/webbpsf/webbpsf-data-source/WFI/sources/
and /itar/jwst/tel/share/webbpsf/webbpsf-data-source/
.
Issue by josePhoenix Monday Aug 17, 2015 at 15:52 GMT Originally opened as https://github.com/mperrin/webbpsf/pull/75
Here's the WFI field dependence code. It depends on changes in POPPY and in the
webbpsf-data
files.In POPPY, the changes are to select OPD units based on the
BUNIT
header keyword and to add a hook withingetOpticalSystem
to add aberration optics (get_aberrations
). (https://github.com/mperrin/poppy/pull/105)In the
webbpsf-data
files, the changes are to addBUNIT = 'micron'
to all OPD maps for consistency with the POPPY change.josePhoenix included the following code: https://github.com/mperrin/webbpsf/pull/75/commits