JP-3593: Saturation step should increase flagging for group 3 saturation #8499
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8499 +/- ##
==========================================
+ Coverage 59.54% 59.56% +0.01%
==========================================
Files 391 391
Lines 39292 39310 +18
==========================================
+ Hits 23396 23414 +18
Misses 15896 15896 ☔ View full report in Codecov by Sentry. |
|
@penaguerrero Now that this update has been moved over to the stcal repo, can this PR be closed? |
|
@penaguerrero What's the relationship between this PR and spacetelescope/stcal#259, which implements the saturation check in the part of the code that resides in stcal? Can this PR be closed, because it's no longer necessary? Or will we need at least some minimal change in the top-level saturation_step module here in jwst to add anything like a new step parameter? |
penaguerrero
changed the title
|
The PR is necessary because due to the changes on stcal, which adds code to an already present parameter, the jwst side needs to now use that parameter. This PR makes use of this parameter as well as modifying the tests. |
jwst/saturation/saturation.py
Outdated
|
|
||
| # Identify groups that are saturated in the third group | ||
| dq_temp = x_irs2.from_irs2(groupdq[ints, 2, :, :], irs2_mask, detector) | ||
| gp3mask = np.where(dq_temp & SATURATED, True, False) |
There was a problem hiding this comment.
This code won't work here; it's looking for where SATURATED is set in groupdq for group==2, but the flags aren't passed from flag_temp into groupdq until after this code block executes. Likewise for the stcal PR.
jwst/saturation/saturation.py
Outdated
| if group == 2: | ||
| # Identify groups which we wouldn't expect to saturate by the third group, | ||
| # on the basis of the first group | ||
| mask = sci_temp[ints, 0, ...] / np.mean(read_pattern[0]) * read_pattern[2][-1] < sat_thresh |
There was a problem hiding this comment.
I don't think this will work; sci_temp as defined above is 2048x2048, so accessing sci_temp[ints,0,...] will just return a single element [0,0] of the array.
jwst/saturation/saturation.py
Outdated
| mask = sci_temp[ints, 0, ...] / np.mean(read_pattern[0]) * read_pattern[2][-1] < sat_thresh | ||
|
|
||
| # Identify groups with suspiciously large values in the second group | ||
| mask &= sci_temp[ints, 1, ...] > sat_thresh / len(read_pattern[1]) |
There was a problem hiding this comment.
Likewise here, this will just return element [0,1] of the 2d sci_temp array.
There was a problem hiding this comment.
I think we also need something to grow this mask by n_pix_grow_sat using the adjacency_sat routine. For pixels bordering pixels that saturate in group 3, the saturation flag is set too. Those pixels might not be intrinsically bright enough to trigger the 'suspiciously large' criterion, but will likewise be affected by the CR and restricted to only fitting a ramp to groups 1+2.
There was a problem hiding this comment.
@drlaw1558 thank you for the comments! I'm committing the changes soon. Can I please ask you to take look again when they are in on this branch as well?
jwst/saturation/saturation.py
Outdated
|
|
||
| # now, flag any pixels that border saturated pixels | ||
| if n_pix_grow_sat > 0: | ||
| mask = adjacency_sat(mask, SATURATED, n_pix_grow_sat) |
There was a problem hiding this comment.
I don't think we can call it like this; 'mask' is an array of True/False whereas adjacency_sat expects input of actual dq flag values. It should work though to say
if n_pix_grow_sat > 0:
dq_temp = adjacency_sat(dq_temp, SATURATED, n_pix_grow_sat)
right after line 188 before the call to x_irs2.to_irs2
| # Flag the 2nd group for the pixels passing that gauntlet | ||
| dq_temp = x_irs2.from_irs2(groupdq[ints, 1, :, :], irs2_mask, detector) | ||
| dq_temp[mask] |= SATURATED | ||
| x_irs2.to_irs2(flagarray, dq_temp, irs2_mask, detector) |
There was a problem hiding this comment.
I don't think this is being passed back correctly; we're populating flagarray, but flagarray only gets put into the groupdq array a few lines later for group==2 whereas we're trying to set saturation bits for group==1.
There was a problem hiding this comment.
@drlaw1558 I made some changes and I believe you were correct and now it sets the flags ok, can you verify please?
| mask &= gp3mask | ||
|
|
||
| # Flag the 2nd group for the pixels passing that gauntlet | ||
| dq_temp = x_irs2.from_irs2(groupdq[ints, 1, :, :], irs2_mask, detector) |
There was a problem hiding this comment.
Nearly there, but I think this line is problematic. Since it's using the existing groupdq array, it's going to be growing ALL pixels flagged as SATURATED, not just those that passed the gauntlet above, so things in group1 will get grown twice.
I think this should be possible to deal with by replacing
dq_temp = x_irs2.from_irs2(groupdq[ints, 1, :, :], irs2_mask, detector)
with
dq_temp = np.zeros_like(mask).astype(int)
There was a problem hiding this comment.
I suppose it would probably be better form though to explicitly cast the dq_temp to be the same dtype as groupdq.
|
Hm, I'm also seeing some unintended consequences on NIRISS data from this PR. Defining the read_patt is activating dilution_factor code in stcal that wasn't being used before, resulting in larger-than-expected flagging. Digging into this further. |
|
Ok, I'm inclined to say that we should remove the changes around line 73-76 in this jwst PR, and not merge the stcal PR at this time. This should limit the changes to affecting NIRSpec IRS2 data only for the time being, and fix the immediate issue while giving us more time to study what's going on with the NIRISS data. |
|
closing this PR as a new branch is dealing with the issues found for NIRISS and NIRCam data https://github.com/drlaw1558/stcal/tree/jp3593_take2. |
Resolves JP-3593
Closes #8413
This PR adds a check for saturation bias in group 2 for nframes > 1.
Checklist for PR authors (skip items if you don't have permissions or they are not applicable)
CHANGES.rstwithin the relevant release sectionHow to run regression tests on a PR