spacetelescope / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
570 stars 167 forks source link

JP-3727: changed psfalign product from quadmodel to cubemodel #8747

Closed emolter closed 2 months ago

emolter commented 2 months ago

Resolves JP-3727

Closes #8746

A previous pull request implemented a change that simplified the pipeline's coronagraphic data processing such that the alignment of the PSF to the science data is only done on the first science integration, then assumed to be the same for all subsequent integrations. Prior to this change, the shifted PSF was stored in a 4-dimensional data structure (QuadModel of shape n_sci_integrations, n_psf_integrations, n_rows, n_cols), which made sense when the shift was different on each science integration. Now, though, this model is identical over the zeroth axis and therefore redundant with itself. Furthermore, this model uses a lot of memory (see JP-3724), and is likely a (the?) cause of the out-of-memory failures reported on multiple datasets in JWSTDMS-921 (see comments on that ticket for additional datasets). This PR stores the shifted PSF as a CubeModel instead.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

emolter commented 2 months ago

regression tests started here

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 60.63%. Comparing base (fb1a6bc) to head (8381a26). Report is 5 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8747 +/- ## ========================================== - Coverage 60.63% 60.63% -0.01% ========================================== Files 372 372 Lines 38375 38372 -3 ========================================== - Hits 23270 23267 -3 Misses 15105 15105 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

emolter commented 2 months ago

Regression tests are all accounted for: 18 of the 22 failures match the ones on the nightly runs here. The other four (two each for NIRCam and MIRI) are all directly testing the psfalign product, which with this PR changed from a QuadModel to a CubeModel, so these failures are expected.. The _i2d and other products output by coron3 are all identical.

mperrin commented 2 months ago

I am going to defer or hand off doing a review for this one, as I'm not an INS coronagraphy folks anymore. I would suggest that instead @aggle should review and concur for MIRI, and @juliengirard for NIRCam.

aggle commented 2 months ago

Looks good to me as well!

melanieclarke commented 2 months ago

Okay, sounds like we have agreement. I'll go ahead and merge. Thanks all!