-
Notifications
You must be signed in to change notification settings - Fork 169
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-3593: 2nd group saturation part 2 #8731
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8731 +/- ##
=======================================
Coverage 61.75% 61.76%
=======================================
Files 377 377
Lines 38750 38764 +14
=======================================
+ Hits 23931 23943 +12
- Misses 14819 14821 +2 ☔ View full report in Codecov by Sentry. |
@tapastro Should now be ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, given that Roman is happy with the corresponding changes in stcal, and given your testing across multiple instrument modes. Thanks for the unit tests and documentation updates!
One small documentation request below, for the new function parameters.
I will run regression tests when the system is free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! One last small formatting thing - the type annotation should be the Python type (bool).
I started regression tests here:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1709/
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the regtests, I see a difference for NIRISS rate files, which I think is because the corresponding stcal branch is out of date - the difference looks related to the chargeloss readnoise variance calculation change.
I also see one difference for a NIRSpec rate file, to the DQ plane only. It looks like some pixels that were do not use but not flagged as saturated are now also flagged as saturated. Looking at the image in a local run, it certainly looks like they were saturated, so this is a reasonable change.
I think this is good to go from my perspective, pending the merge of the corresponding stcal PR. @tapastro - can you please coordinate on the stcal side?
Ready for review: this addresses JP-3593, flagging cosmic ray saturation in group 2 data in a consistent manner between both IRS2 and non-IRS2 modes using the new use_readpatt keyword. This PR is tied to STCAL PR spacetelescope/stcal#283
Checklist for PR authors (skip items if you don't have permissions or they are not applicable)
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR