Roman Cycle 9 Updates #466

ojustino · 2021-06-24T22:14:44Z

Would resolve #465.

Updates

`webbpsf`

`WFIPupilController`

Added pupil_file_formatters attribute that will hopefully allow for easier updates when pupil filename structures change in future cycles.
Created update_pupil() from the remains of the former _update_pupil(), update_pupil_path(), and validate_pupil(). This method points WFI to the appropriate pupil file and pupil mask string after the user changes WFI's detector or filter. It is now the only standard approach for updating the pupil or pupil mask.
- Added _get_filter_mask() to help match a given filter with its corresponding pupil mask.
As such, locked pupil_mask and pupil so that direct assignment is no longer possible, though _pupil and _pupil_mask allow for indirect assignment in update_pupil().
Added lock_pupil() and lock_pupil_mask() methods to disable default pupil/mask selection behavior by toggling the new _auto_pupil and _auto_pupil_mask attributes to False, respectively. Instead, they lock WFI on the provided pupil file or pupil mask string. unlock_pupil() and unlock_pupil_mask() re-enable default behavior.
Removed all (un)masked pupil-related attributes as there's no such distinction in the new pupil files.
~~GRISM_FILTER~~ GRISM_FILTERS and ~~PRISM_FILTER~~ PRISM_FILTERS are now tuples containing the new string labels for those filters in filters.tsv (including separate entries for grism's 0th and 1st degrees).

`WFI`

Pointed ~~_aberrations_files~~ _aberration_files attribute to Cycle 9's Zernike files.
- Renamed ~~_is_custom_aberrations~~ _is_custom_aberration and ~~_current_aberrations_file~~ _current_aberration_file.
Removed _unmasked_pupil_path and _masked_pupil_path since their corresponding attributes were deleted from WFIPupilController.
Renamed ~~override_aberrations()~~ lock_aberrations() and ~~reset_override_aberrations()~~ unlock_aberrations().
Created new lock_pupil() and lock_pupil_mask() methods for advanced users who prefer to disable automatic pupil/mask selection and instead stick with a specific pupil file or mask, respectively. The corresponding unlock_pupil() and unlock_pupil_mask() methods return the class to its normal behavior. (These methods follow in the footsteps of lock_aberrations() and unlock_aberrations().)
As such, locked pupil and pupil_mask so that direct assignment is no longer possible. Both attributes continue to dynamically update on any change to detector or filter that necessitates it.
Added _update_pupil() as a shortcut to WFIPupilController.update_pupil() used in the lock/unlock methods and the detector/filter setters.

`RomanInstrument`

Updated jitter_sigma to 0.012 arcsec/axis to reflect new simulations (fixes Update Roman Jitter value #443).

Other

Made corresponding changes to Roman test and documentation files.

webbpsf-data-source

(webbpsf-data/ ➡️ webbpsf-data-source/ after #416)

Out with the old...

Archived Cycle 8 Zernike files
- webbpsf-data-source/WFI/wim_zernikes_cycle8.csv ➡️ webbpsf-data-source/WFI/sources/cycle8/wim_zernikes_cycle8.csv
- webbpsf-data-source/WFI/wim_zernikes_cycle8_grism.csv ➡️ webbpsf-data-source/WFI/sources/cycle8/wim_zernikes_cycle8_grism.csv
- webbpsf-data-source/WFI/wim_zernikes_cycle8_prism.csv ➡️ webbpsf-data-source/WFI/sources/cycle8/wim_zernikes_cycle8_prism.csv
Archived previous cycle's throughput-by-filter files (webbpsf-data-source/WFI/filters/*_throughput.fits ➡️ webbpsf-data-source/WFI/sources/cycle8/filters/*_throughput.fits)
Archived previous cycle's pupil files (webbpsf-data-source/WFI/pupils ➡️ webbpsf-data-source/WFI/sources/cycle8/new_pupils)
(Previous cycle's post-grism/prism update version of filters.tsv already archived in webbpsf-data-source/WFI/sources/cycle8_prism_grism/filters.tsv)
(Previous cycle's HST OPD file already archived in webbpsf-data-source/WFI/sources/cycle8/new_upscaled_HST_OPD.fits)

...in with the new.

(To examine file processing procedures, see webbpsf-data-source/WFI/sources/cycle9/Process_zern_and_filters.ipynb for Zernikes and filter files or webbpsf-data-source/WFI/sources/cycle9/Oversample_rescale_pupil_images_cycle_9.ipynb for pupil files.)

Updated Zernike files (note change from wim to wsm in spectral mode filenames to better distinguish them from imaging mode)
- webbpsf-data-source/WFI/sources/cycle9/wim_zernikes_cycle9.csv
  - (Needs to be sorted by SCA for some reason?)
- webbpsf-data-source/WFI/sources/cycle9/wsm_zernikes_cycle9_grism.csv
- webbpsf-data-source/WFI/sources/cycle9/wsm_zernikes_cycle9_prism.csv
Added new imaging filter and renamed prism/grism filters as described in next section (webbpsf-data-source/WFI/sources/cycle9/filters.tsv)
Created filter throughput files (webbpsf-data-source/WFI/sources/cycle9/filters/*_throughput.fits)
Updated OPD file to match new pupil images' pixel scale (webbpsf-data-source/WFI/sources/cycle9/new_upscaled_HST_OPD.fits)

The files and directories were "pre-archived" in webbpsf-data-source/WFI/sources/cycle9/ and then copied to their appropriate locations in webbpsf-data-source (the renamed upscaled_HST_OPD.fits) or webbpsf-data-source/WFI/ (the rest).

Checks

Notable Roman design changes

As seen above, the wavelength range now covers 0.48 to 2.3 micrometers.
- The new F213 filter adds coverage from 1.95 to 2.3 micrometers.
The grism mode is now split into an undispersed zeroth order and dispersed first order.
Sampling points are no longer four pixels from each corner.

codecov · 2021-08-02T02:56:28Z

Codecov Report

Merging #466 (cd834eb) into develop (3489d38) will increase coverage by 0.02%.
The diff coverage is 90.22%.

@@             Coverage Diff             @@
##           develop     #466      +/-   ##
===========================================
+ Coverage    62.86%   62.89%   +0.02%     
===========================================
  Files           11       11              
  Lines         4616     4622       +6     
===========================================
+ Hits          2902     2907       +5     
- Misses        1714     1715       +1

Impacted Files	Coverage Δ
webbpsf/webbpsf_core.py	`81.12% <ø> (ø)`
webbpsf/roman.py	`90.63% <90.22%> (-0.09%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3489d38...cd834eb. Read the comment docs.

Skyhawk172 · 2021-10-22T00:27:54Z

webbpsf/roman.py

-        self._filter = value
+        # UNRESOLVED: how do we know which pupil to reset to? make it clear in the docstring that you're unlocking *and* resetting to default.
+        # UNRESOLVED: if you unlock_pupil_mask, lock_pupil, unlock_pupil, does the pupil mask also reset?
+        """


It appears like this is doing the right thing, mainly because of the call to self._update_pupil(). The reset is based on the current values of wfi.filter and wfi.detector.

Agreed, thanks for the sanity check.

Skyhawk172 · 2021-10-22T00:28:25Z

webbpsf/roman.py

-        # Update Pupil
-        # ------------
-        self._pupil_controller.validate_pupil(self._filter)
+        UNRESOLVED: how do we know which mask to reset to?


Same as above (line 815). The reset is based on the current values of wfi.filter and wfi.detector, and they seem to behave as expected.

Skyhawk172 · 2021-10-22T00:29:51Z

webbpsf/roman.py

+    @pupil.setter
+    def pupil(self, value):
+        # check if we've been through super() in init
+        # UNRESOLVED: the check could be more explicit...


I'll also defer to your best programmer judgment here.

Skyhawk172 · 2021-10-22T00:30:20Z

webbpsf/roman.py

-
-    @pupil_mask.setter
-    def pupil_mask(self, name):
+        UNRESOLVED: better double-checking strategy than assert?


I'll defer to your best programmer judgment here.

I don't think these are wrong, but a better alternative isn't coming to me. I'll leave it as is for the sake of time.

Skyhawk172 · 2021-10-22T00:52:55Z

webbpsf/roman.py

-        _log.info("Removing custom pupil mask")
-        self.pupil_mask = 'AUTO'
+        UNRESOLVED: how do we know which mask to reset to?
+        """


Limited understanding of the class here, so bear with me. I'm not sure if there's a use case where this particular method gets called.

From what I see, the locking/unlocking of the pupil and pupil_mask is done on the WFI object, not the WFIPupilController object. The former seems to be handling the unlocking as expected because it knows the values of the filter and detector, and resets them using wfi._update_pupil().

But WFIPupilController, by itself, has no knowledge of these values, so it cannot properly reset the pupil and pupil_mask. When we unlock the pupil, all it does is set the auto_lock to True, leaving the pupil and pupil_mask values as they were.

When instantiated, the pupil and pupil_mask are set to None - maybe that's what they should be reset to.

You're right in saying that WFIPupilController has no knowledge of the current pupil or pupil mask value and that the actual assignment of the pupil_mask attribute happens in WFI. This note was a reminder to re-check the WFI version of unlock_pupil_mask() since it calls this one.

Skyhawk172 · 2021-10-22T01:10:43Z

webbpsf/tests/test_roman.py

-        wi.filter = filter
-        wi.calc_psf(fov_pixels=4, oversample=1, nlambda=3)
+        if filter == 'GRISM0':
+            # UNRESOLVED: GRISM0 errors out. poppy's Instrument._get_weights()


As I mentioned on Slack, I'm able to run calc_psf() with GRISM0, so I'm not sure what's happening here. This particular test is passing for me even when I remove your IF GRISM0 statement:

test_roman.py::test_WFI_filters PASSED [100%]

======================== PASSES ========================
------------------------------- Captured stdout call -----------------------
F062
F087
F106
F129
F146
F158
F184
PRISM
F213
GRISM0
GRISM1
====================== 1 passed in 33.75s ===================

This is the only comment that I'd say requires a follow up. calc_psf() with GRISM0 errors out for me even with a fresh install and a new environment, and I think the failure makes sense given what I commented in the code about get_weights() in poppy.

What do you get when you try the following (replacing the ellipsis with your path to the new data files)?

from astropy.io import fits any(fits.getdata('.../WFI/filters/GRISM0_throughput.fits')['THROUGHPUT'] > .1)

If it's False, I'm not sure why poppy would accept the weights as valid.

mperrin · 2021-12-08T16:11:26Z

docs/roman.rst

@@ -73,7 +73,7 @@ The WFI field of view is laid out as shown in the figure. To select a different
   ['SCA01', 'SCA02', 'SCA03', 'SCA04', 'SCA05', 'SCA06', 'SCA07', 'SCA08', 'SCA09', 'SCA10', 'SCA11', 'SCA12', 'SCA13', 'SCA14', 'SCA15', 'SCA16', 'SCA17', 'SCA18']
   >>> wfi.detector = 'SCA03'

-The usable region of the 4096 by 4096 pixel detectors specified for the Wide Field Instrument will range from (4, 4) to (4092, 4092), accounting for the 4 pixel wide bands of reference pixels. **[The preceding may no longer be true.]** To change the position at which to calculate a PSF, simply assign an (X, Y) tuple::
+The usable regions of the Wide Field Instrument's detectors are slightly smaller than their 4096 by 4096 pixel dimensions due to variations in reference pixel coverage at their edges. To change the position at which to calculate a PSF, assign an (X, Y) tuple::


This text does not seem correct; the reference pixels don't vary. It's a fixed width of 4 pixels around all edges of all detectors. Rephrase to something like the following.

The usable, photosensitive regions of the Wide Field Instrument's detectors are slightly smaller than their 4096 by 4096 pixel dimensions, because the outermost 4 rows and columns are reference pixels that are not sensitive to light.

But that said, I don't understand entirely why this is relevant here for the PSF calculations. This is a detector property, not something in the optical models.

I talked about it with Charles but will catch up with you about this.

mperrin · 2021-12-08T16:13:36Z

webbpsf/roman.py

-
-        # UNRESOLVED: how do we know which pupil to reset to? make it clear in the docstring that you're unlocking *and* resetting to default.
-        # UNRESOLVED: if you unlock_pupil_mask, lock_pupil, unlock_pupil, does the pupil mask also reset?
+        Undoes the effects of lock_pupil() by resetting the class to


This is out of scope for this PR, but at some point it might be worth revisiting some of the complexity that's been added into the WFI classes, such as this lock and unlock pupil functionality. What is the use case for that? Is it still needed? Does anyone actually use this part of the code? I think it may be the case that some of the code added over time is no longer particularly useful, and could potentially be simplified out.

Robel and I spoke with Adric about this and Adric said it's useful in his workflows for the Roman ETC (and perhaps Pandeia). Robel also told me that there are some other, more intrepid users who like having this functionality.

ojustino · 2021-12-08T16:25:42Z

@mperrin figured out that the GRISM0 discrepancy happened because poppy's Instrument._get_weights() works differently depending on whether or not synphot is present. synphot is currently optional but will become a requirement in WebbPSF 1.0.

I've still left GRISM0 out of test_WFI_filters() to keep this pull request's tests passing. I'll create an issue to reverse that once the new release is out.

mperrin · 2021-12-08T16:30:34Z

@ojustino This looks good, apart from one quibble with some doc string text. We can fix that separately as part of other docs edits for release 1.0. I'm going to go ahead and merge this now.

We ought to be able to add GRISM0 into the tests immediately, since once this is merged into develop the tests will run with synphot installed. So let's try immediately making a PR to reverse that right away after I merge this.

ojustino force-pushed the cycle9-base branch from 28d4c6a to f244f33 Compare June 24, 2021 22:16

robelgeda added Roman Affects Roman Space Telescope models in WebbPSF WIP labels Jun 25, 2021

ojustino force-pushed the cycle9-base branch from f244f33 to fb4cde8 Compare June 30, 2021 17:45

ojustino force-pushed the cycle9-base branch 3 times, most recently from 1102d85 to 75a80c6 Compare July 15, 2021 21:03

ojustino requested a review from Skyhawk172 July 20, 2021 17:34

ojustino force-pushed the cycle9-base branch 2 times, most recently from fc73761 to 8f35223 Compare August 2, 2021 02:43

mperrin assigned ojustino Sep 7, 2021

Skyhawk172 approved these changes Oct 25, 2021

View reviewed changes

ojustino force-pushed the cycle9-base branch 2 times, most recently from 8c51443 to c75a455 Compare November 19, 2021 04:48

ojustino added 12 commits December 3, 2021 13:50

Pointed Roman class to Cycle 9 aberration files

ce4a303

Made WFIPupilController updates for Cycle 9

82a9d9c

Fixed a copy/paste error

631dcdc

Fixed WFIPupilController's file name constructor

436a7f4

Pointed tests to new source files for v1.0.0.rc

b4ec0b6

Beautified WFI code; renamed grism/prism filters

0ac031c

Updated RomanInstrument's jitter value

d7fe153

Began update of Roman tests

8f6ef45

Roman: Updated docs for Cycle 9; cleaned up tests

071edfb

Implemented Skyhawk172's reference frame fix

2cbea6c

Addressed most of Skyhawk172's review comments

a4641a1

Updated WFI interpolation limit test

cd834eb

ojustino force-pushed the cycle9-base branch from c75a455 to cd834eb Compare December 3, 2021 23:08

mperrin reviewed Dec 8, 2021

View reviewed changes

mperrin merged commit b996bcd into spacetelescope:develop Dec 8, 2021

This was referenced Dec 8, 2021

Added GRISM0 back to WFI filter tests #508

Merged

Release prep for 1.0.0 #507

Merged

Added Roman updates to v1.0.0 release notes #514

Merged

Corrected info on Roman reference pixels #515

Merged

ojustino mentioned this pull request Jun 2, 2023

Roman Cycle 9 Reference Data file corrupted #666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roman Cycle 9 Updates #466

Roman Cycle 9 Updates #466

ojustino commented Jun 24, 2021 •

edited

Loading

codecov bot commented Aug 2, 2021 •

edited

Loading

Skyhawk172 Oct 22, 2021

ojustino Nov 18, 2021

Skyhawk172 Oct 22, 2021

Skyhawk172 Oct 22, 2021

Skyhawk172 Oct 22, 2021

ojustino Nov 18, 2021

Skyhawk172 Oct 22, 2021

ojustino Nov 18, 2021

Skyhawk172 Oct 22, 2021

ojustino Nov 18, 2021

mperrin Dec 8, 2021

ojustino Dec 8, 2021

mperrin Dec 8, 2021

ojustino Dec 8, 2021 •

edited

Loading

ojustino commented Dec 8, 2021

mperrin commented Dec 8, 2021

Roman Cycle 9 Updates #466

Roman Cycle 9 Updates #466

Conversation

ojustino commented Jun 24, 2021 • edited Loading

Updates

webbpsf

WFIPupilController

WFI

RomanInstrument

Other

webbpsf-data-source

Out with the old...

...in with the new.

Checks

Notable Roman design changes

codecov bot commented Aug 2, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojustino Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

ojustino commented Dec 8, 2021

mperrin commented Dec 8, 2021

ojustino commented Jun 24, 2021 •

edited

Loading

`webbpsf`

`WFIPupilController`

`WFI`

`RomanInstrument`

codecov bot commented Aug 2, 2021 •

edited

Loading

ojustino Dec 8, 2021 •

edited

Loading