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-2922: Minor fixes for MSA source names #8533

Closed melanieclarke closed 5 months ago

melanieclarke commented 5 months ago

Resolves JP-2922

This PR addresses two minor issues with PR #8442.

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

melanieclarke commented 5 months ago

Regression tests started here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1492/

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 58.02%. Comparing base (4179c09) to head (e2fb95f). Report is 321 commits behind head on master.

Files with missing lines Patch % Lines
...st/master_background/master_background_mos_step.py 0.00% 1 Missing :warning:
jwst/master_background/nirspec_utils.py 80.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8533 +/- ## ========================================== + Coverage 57.97% 58.02% +0.04% ========================================== Files 387 389 +2 Lines 38830 38962 +132 ========================================== + Hits 22513 22609 +96 - Misses 16317 16353 +36 ```

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

hbushouse commented 5 months ago

@melanieclarke The two places that also need fixing, in order to account for the new way background slits are labeled, are here: https://github.com/spacetelescope/jwst/blob/master/jwst/master_background/master_background_mos_step.py#L194 and here: https://github.com/spacetelescope/jwst/blob/master/jwst/master_background/nirspec_utils.py#L104

They both try to identify background slits by testing for the string "background" in slit.source_name, but the last changes to the slit handling now use the string "BKG" in the source name and alias. So at minimum these if statements need to be updated to look for "BKG" in slit.source_name. Furthermore, it seems inefficient to have this same check done in two different places in two different code modules (makes maintenance that much more of a burden). So I'm wondering if you could somehow make a single "is_background" type of utility function in one module or the other and then just call the same one from both places.

melanieclarke commented 5 months ago

Thanks @hbushouse - I independently found those exact two lines yesterday afternoon. I'll add a fix today.

braingram commented 5 months ago

Does this also fix the currently failing test_nirspec_mos_mbkg regtest? https://plwishmaster.stsci.edu:8081/job/RT/job/JWST/2914/testReport/junit/jwst.regtest/test_nirspec_masterbackground/_stable_deps__test_nirspec_mos_mbkg_masterbg1d_/ Running the failing test with -s shows the following in the log:

2024-06-03 13:31:01,638 - stpipe.Spec2Pipeline.master_background_mos - INFO - Step master_background_mos running with args (<MultiSlitModel from jw01180025001_05101_00001_nrs2_rate.fits>,).
2024-06-03 13:31:01,656 - stpipe.Spec2Pipeline.master_background_mos - WARNING - No background slits available for creating master background. Skipping
melanieclarke commented 5 months ago

@braingram - yes, that's the symptom. I'll check that test locally when I have the fix.

melanieclarke commented 5 months ago

Fix pushed. Running jwst/regtest/test_nirspec_masterbackground.py locally still shows failures, because there are changes from the last truth data, but it no longer fails because the *_masterbg2d.fits file cannot be found. The master_background_mos step now runs.

Regression tests restarted here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1493/

melanieclarke commented 5 months ago

Regression tests show expected differences for the master background test and some unrelated differences for MIRI LRS spec3.