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-3702: Fix filenames for level3 NIRSpec #8699

Merged
merged 9 commits into from
Aug 27, 2024

Conversation

melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Aug 9, 2024

Resolves JP-3702

Closes #8688

Restore slit names to level 3 filenames for NIRSpec products that do not get planned as fixed slit reductions. This primarily impacts BOTS mode, which has a FXD_SLIT defined, but does not get planned for spec3 with Asn_Lv3NRSFSS rules: these data are planned for tso3 with Asn_Lv3TSO rules. It also impacts background targets for fixed slit, planned with Asn_Lv3SpecAux rules.

In JP-3233 (#7879), association rules were changed for fixed slits to allow S200A1 and S200A2 targets to be reduced together in spec3. Part of this change removed a check for fixed slit as one of the 'opt_elem' fields in the filename. This had the unintended effect of removing the fixed slit from the name for BOTS and auxiliary NIRSpec data.

The fix here is to restore the check for FXD_SLIT in all level3 product names that might apply to NIRSpec data and add the value to the filename if present.

It looks like this regression was reported in the regression tests for JP-3233, e.g. the last entry in this report:
test_against_standard_pool_010_spec_nirspec_lv2bkg, but it was buried in some other expected changes. There were also some unit tests for level3 product names, but they were not specific enough to catch this regression.

I have updated the unit tests (test_level3_product_names.py) to catch this case specifically, and verified that it fails on master and passes with this PR branch. I think the regression tests should be sufficient as is.

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

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.35%. Comparing base (53cabe4) to head (99ffcc9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8699      +/-   ##
==========================================
+ Coverage   60.33%   60.35%   +0.01%     
==========================================
  Files         372      372              
  Lines       38361    38370       +9     
==========================================
+ Hits        23145    23157      +12     
+ Misses      15216    15213       -3     

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


🚨 Try these New Features:

@melanieclarke
Copy link
Collaborator Author

@melanieclarke
Copy link
Collaborator Author

Regression tests look correct. Only NIRSpec associations are affected, and the only change in each case is that the product name now contains the slit, in between the grating and the subarray (if present).

@melanieclarke melanieclarke marked this pull request as ready for review August 12, 2024 14:41
@melanieclarke melanieclarke requested a review from a team as a code owner August 12, 2024 14:41
@melanieclarke melanieclarke requested a review from tapastro August 12, 2024 14:41
Copy link
Contributor

@tapastro tapastro left a comment

Choose a reason for hiding this comment

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

It looks like the pool 010 regression test has a duplicated slitname entry in the product name. Other than that, this looks good to me.

@melanieclarke
Copy link
Collaborator Author

It looks like the pool 010 regression test has a duplicated slitname entry in the product name. Other than that, this looks good to me.

I saw that too -- it's actually just because of some weird input data in the synthetic pools. The slit and subarray are set to the same value, and it is appropriately listing them both.

@tapastro tapastro self-requested a review August 26, 2024 16:53
Copy link
Contributor

@tapastro tapastro left a comment

Choose a reason for hiding this comment

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

Hmm, it may be time to investigate covering our association testing through flight pools over the synthetic pools...

LGTM!

@melanieclarke melanieclarke merged commit 582dbd9 into spacetelescope:master Aug 27, 2024
28 of 29 checks passed
@melanieclarke melanieclarke deleted the jp-3702 branch August 27, 2024 13:06
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.

NIRSpec TSO filenames may be missing the slitname
2 participants