spacetelescope / jwst

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

JP-3635: Account for pixel area ratio in outlier detection blotting #8553

Closed emolter closed 3 months ago

emolter commented 3 months ago

Resolves JP-3635

Closes #8509

This PR addresses a small bug in outlier detection: resample accounted for the difference between the nominal pixel area in steradians used for the photometric calibration (img.meta.photometry.pixelarea_steradians) and the as-computed pixel area from the WCS when resampling cal files. That wasn't performed in reverse when blotting median images back to match those individual cal files. This PR applies that reverse ratio so that the total flux is conserved between input models and blotted models.

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

emolter commented 3 months ago

Regression tests started here.

edit: a second regtest run started because the first one had some 100 unrelated failures

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 58.54%. Comparing base (b7e0b10) to head (29cac7c). Report is 361 commits behind head on master.

Files with missing lines Patch % Lines
jwst/outlier_detection/outlier_detection.py 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8553 +/- ## ========================================== + Coverage 58.02% 58.54% +0.52% ========================================== Files 388 388 Lines 38977 38963 -14 ========================================== + Hits 22617 22812 +195 + Misses 16360 16151 -209 ```

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

braingram commented 3 months ago

@mairanteodoro Does romancal need a similar fix?

drlaw1558 commented 3 months ago

Looks good to me; as a test I tried deliberately messing up the header PIXAR keywords for some MIRI data by 20% to really throw off the outlier detection routine. In the image attached the left panel is the original drizzled i2d image (with good PIXAR values), the middle panel is the existing jwst code working on the data with bad PIXAR values, and the right panel is this PR working on the data with bad PIXAR values. As expected the current code does really bad things because the scaling isn't being applied when blotting. This PR looks like it cleans that up really well, and the result looks identical to the original (except, of course, for the fluxcal error that I introduced by hacking the PIXAR keywords).

jp3635
emolter commented 3 months ago

Most of the 80 or so regtest failures stem from ValueError: WCS must have array_shape attribute set. when attempting to do compute_image_pixel_area(blot_img.meta.wcs) in this step. The modes affected are seemingly all NIRSpec modes as well as MIRI LRS. Do we expect the wcs to have an array_shape for these modes?

The array_shape seems to be set by resample_step.get_drizpars() with errors if it cannot be computed. We only attempt to do blotting if resample_data is True. However, the outlier_detection step does not run the entire resample step, calling resample.ResampleData directly instead. So the options seem to be to run get_drizpars from inside this step (not sure what complications might arise), to hard-code pix_ratio to unity if array_shape is not set, or to manually set the array_shape (not sure precisely how to do this yet).

melanieclarke commented 3 months ago

Most of the 80 or so regtest failures stem from ValueError: WCS must have array_shape attribute set. when attempting to do compute_image_pixel_area(blot_img.meta.wcs) in this step. The modes affected are seemingly all NIRSpec modes as well as MIRI LRS. Do we expect the wcs to have an array_shape for these modes?

Currently, spectral data does not have iscale computed/applied in resampling, so it shouldn't be applied in blotting either. You'll need to check for 'SPECTRAL' not in img.meta.wcs.output_frame.axes_type, as is done in the resample module.

emolter commented 3 months ago

started another regtest run with the suggested change

hbushouse commented 3 months ago

@emolter Your latest regtest run is still corrupted with unrelated changes due to 1) new NIRCam photom ref files and 2) changes to the jump step in stcal. The truth files for the NRC photom related tests have been updated now, and a new version of stcal (1.7.2) has been released that contains the jump updates (and truth files updated). So try running again, being sure you're using stcal 1.7.2 (or stcal/main).

emolter commented 3 months ago

@emolter Your latest regtest run is still corrupted with unrelated changes due to 1) new NIRCam photom ref files and 2) changes to the jump step in stcal. The truth files for the NRC photom related tests have been updated now, and a new version of stcal (1.7.2) has been released that contains the jump updates (and truth files updated). So try running again, being sure you're using stcal 1.7.2 (or stcal/main).

started here

edit: There are now 8 failures left, and I believe that they are all expected: I checked the value of pix_ratio in the test_nircam_image and test_image3_closedfile, and found small but potentially non-trivial deviations from unity of 0.997, 1.005, respectively.

The fraction of pixels that differ is <~0.01% for all the i2d files (nircam_image, miri_image, nircam_mtimage, except for test_image3_closedfile which shows a 1% fraction.