Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JP-2922: Minor fixes for MSA source names #8533

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Jun 3, 2024

Resolves JP-2922

This PR addresses two minor issues with PR #8442.

  • Add default fallback values for an edge case in NIRSpec MSA handling: when a source is specified in the SHUTTER_INFO table, but not found in the SOURCE_INFO table. These cases used to be handled as background sources, but background sources now have their own separate handling. This case is unlikely to be encountered in operations; this handling would mostly only be needed to gracefully handle a hand-edited MSA file.
  • Add a function to check whether a slit contains a background source, and update the test from checking for "background" (old-style background source names) to "BKG" (new-style) in the slit.source_name attribute.

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

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • All comments are resolved
  • Make sure the JIRA ticket is resolved properly

@melanieclarke
Copy link
Collaborator Author

Copy link

codecov bot commented Jun 3, 2024

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 ⚠️
jwst/master_background/nirspec_utils.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hbushouse
Copy link
Collaborator

@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
Copy link
Collaborator Author

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

@braingram
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@melanieclarke melanieclarke changed the title JP-2922: Add fallback value for MSA slits not in source table JP-2922: Minor fixes for MSA source names Jun 4, 2024
@melanieclarke
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@melanieclarke melanieclarke marked this pull request as ready for review June 4, 2024 17:26
@melanieclarke melanieclarke requested a review from a team as a code owner June 4, 2024 17:26
@melanieclarke melanieclarke requested a review from hbushouse June 4, 2024 18:04
Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hbushouse hbushouse merged commit ad586d3 into spacetelescope:master Jun 5, 2024
30 checks passed
@melanieclarke melanieclarke deleted the jp-2922 branch June 5, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants