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-3151: Don't make associations for NRS2 IFU if detector not illuminated #8395

Merged
merged 4 commits into from Apr 4, 2024

Conversation

stscirij
Copy link
Contributor

@stscirij stscirij commented Mar 28, 2024

Filter dupes in mkpool.
Add requirement that no spec2 associations are made for NRS IFU settings that don't illuminate the NRS2 detector
Fix test that made such an association

Resolves JP-3151

Closes #7775

This PR addresses remaining issues with this ticket: only data with PATTTYPE with NODs, plus imprint and background exposures was being handled properly, but not all other types. In this PR, all spec2 associations check whether the nrs2 detector is illuminated before creating an association.

Checklist for maintainers

  • 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
  • Make sure the JIRA ticket is resolved properly

Add requirement that no spec2 associations are made for NRS IFU settings
that don't illuminate the NRS2 detector
Fix test that made such an association
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.78%. Comparing base (2fb073e) to head (525710b).
Report is 22 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8395      +/-   ##
==========================================
+ Coverage   75.31%   75.78%   +0.47%     
==========================================
  Files         474      476       +2     
  Lines       38965    39450     +485     
==========================================
+ Hits        29345    29897     +552     
+ Misses       9620     9553      -67     
Flag Coverage Δ *Carryforward flag
nightly 77.65% <ø> (+0.31%) ⬆️ Carriedforward from 7ee0517

*This pull request uses carry forward flags. Click here to find out more.

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

@tapastro
Copy link
Contributor

@hbushouse hbushouse added this to the Build 11.0 milestone Apr 1, 2024
@hbushouse
Copy link
Collaborator

Of the 3 association-related failures in the regtest, it's not obvious from the logs whether the changes are expected due to this update. Can you confirm?

@stscirij
Copy link
Contributor Author

stscirij commented Apr 3, 2024

The 3 regression test failures in the developer PR test above are all because the truth files have NRS_IFU spec2 associations where there is no data on nrs2, so the PR code doesn't make them.

@hbushouse
Copy link
Collaborator

The 3 regression test failures in the developer PR test above are all because the truth files have NRS_IFU spec2 associations where there is no data on nrs2, so the PR code doesn't make them.

Great. So in that case, I hereby approve this PR. ;-)

@hbushouse hbushouse merged commit fa153cc into spacetelescope:master Apr 4, 2024
27 of 28 checks passed
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.

assign_wcs aborts for NRS IFU due to no data on NRS2
3 participants