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-3560: Updating CHARGELOSS Flagging #8336

Merged
merged 3 commits into from
Mar 11, 2024

Conversation

kmacdonald-stsci
Copy link
Contributor

@kmacdonald-stsci kmacdonald-stsci commented Mar 8, 2024

Resolves JP-3560

Closes #8331

This PR addresses CHARGELOSS flagging. The original implementation assumed an integration ramp that has a group above the CHARGELOSS threshold will have all remaining groups in the ramp above the CHARGELOSS threshold, so will also get the CHARGELOSS flag. This is not true. Additionally, the charge migration step flags nearest neighbors, the four pixels to the north, south, east, and west, with CHARGELOSS in the groups flagged in the ramp of interest. All subsequent groups in nearest neighbors also need to be flagged with CHARGELOSS and DO_NOT_USE.

The charge migration step has been changed to find the first group in an integration ramp that is above the CHARGELOSS threshold and not flagged as DO_NOT_USE, then it flags that group and all subsequent groups in the ramp with CHARGELOSS and DO_NOT_USE. The charge migration step also flags nearest neighbors in the same groups.

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

…Updated tests to satisfy this new flagging pattern.
@kmacdonald-stsci kmacdonald-stsci requested a review from a team as a code owner March 8, 2024 21:34
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.27%. Comparing base (4cc0ac1) to head (e66abdc).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8336      +/-   ##
==========================================
+ Coverage   75.15%   75.27%   +0.11%     
==========================================
  Files         470      474       +4     
  Lines       38604    39112     +508     
==========================================
+ Hits        29014    29441     +427     
- Misses       9590     9671      +81     
Flag Coverage Δ *Carryforward flag
nightly 77.33% <ø> (-0.07%) ⬇️ Carriedforward from de5d2b6

*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.

@kmacdonald-stsci
Copy link
Contributor Author

@hbushouse hbushouse added this to the Build 10.2 milestone Mar 11, 2024
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.

Regression test run shows a bunch of unrelated differences in NIRSpec results due to a recent CRDS ref file update, as well as the expected differences in NIRISS tests. So that all looks OK.

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.

CHARGELOSS should be propagated to all subsequent groups in a ramp.
3 participants