JP-3593: Saturation step should increase flagging for group 3 saturation #259

penaguerrero · 2024-05-21T20:08:57Z

Resolves JP-3593

Closes spacetelescope/jwst#8413

This PR adds a check for saturation bias in group 2 for nframes > 1.

Checklist

added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
updated relevant tests
updated relevant documentation
updated relevant milestone(s)
added relevant label(s)

codecov · 2024-05-22T16:57:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.81%. Comparing base (5772579) to head (2d32245).
Report is 193 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #259      +/-   ##
==========================================
+ Coverage   83.79%   83.81%   +0.01%     
==========================================
  Files          35       35              
  Lines        6998     7006       +8     
==========================================
+ Hits         5864     5872       +8     
  Misses       1134     1134

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

kmacdonald-stsci

The "resolves" links in the description of the PR should be changed to JP-3593, instead of JP-nnnn, so it links directly to the JP ticket. The RCAL-nnnn should be removed.

Other than that, everything looks good to me.

braingram · 2024-05-22T17:27:33Z

Is there a RCAL ticket? The romancal tests show some failures:
https://github.com/spacetelescope/stcal/actions/runs/9195566204/job/25291696183?pr=259#step:10:1072

@schlafly is there someone from roman who can look at these changes?

schlafly · 2024-05-22T18:41:35Z

Thanks, yes, romancal will definitely need to be changed to accommodate this. "number of frames per group" isn't really a good concept for Roman since it can vary from group to group. I'm having trouble immediately understanding how group 3 is important here. It's not immediately clear to me that we would want this feature in Roman. Can you provide a little more context as to what the purpose is? If we wanted to not change the API, would an option be setting nframes = None by default, and only doing the new block if nframes is not None?

schlafly · 2024-05-22T18:51:37Z

Sorry, now reading the linked https://jira.stsci.edu/browse/JP-3593, I understand the motivation here.

If I have understood correctly, the issue is that the ramp actually is saturating in group 3 due to a very large CR. In Roman we address this via this dilution factor approach here:
https://github.com/spacetelescope/stcal/blob/main/src/stcal/saturation/saturation.py#L87-L93
That basically turns down the saturation threshold for all groups according to how many reads were in the group to account for dilution of the saturated read among the other unsaturated reads within each group. I think you could use that approach productively for Webb though it's different than the approach suggested in JP-3953. I'd suggest we either make this extra nframes argument optional and Roman wouldn't use it, or we follow up in 3953 about whether the dilution factor approach would work there.

hbushouse

I was under the impression that this new flagging of group 2 (when group 3 is saturated) would only occur when nframes>1. It appears to me that nframes is only ever used to compute the values in "read_pattern", but is not used anywhere in this code to trigger the new algorithm. So does that mean it's going to get applied to all exposures, including those with nframes=1?

hbushouse · 2024-06-05T21:17:33Z

src/stcal/saturation/saturation.py

+                    mask &= np.where(gdq[ints, 2, :, :] & saturated, True, False)
+
+                    # Flag the 2nd group for the pixels passing that gauntlet
+                    gdq[ints, 1][mask] |= saturated


Does the "|" (or) operator really work as desired in this case? Because we track flags in individual bits, don't we need to use "np.bitwise_or()" here?

src/stcal/saturation/saturation.py

schlafly · 2024-06-06T01:19:56Z

Re only triggering when nframes > 1, the reason this works is that when nframes = 1, len(read_pattern[1]) = 1, so the bit on line 93 becomes just a cut against the normal saturation level. As you say, it runs even when nframes = 1, but won't flag anything extra in that case. You could code around it to avoid the checks entirely if wanted.

hbushouse · 2024-06-06T12:02:47Z

Re only triggering when nframes > 1, the reason this works is that when nframes = 1, len(read_pattern[1]) = 1, so the bit on line 93 becomes just a cut against the normal saturation level. As you say, it runs even when nframes = 1, but won't flag anything extra in that case. You could code around it to avoid the checks entirely if wanted.

Thanks for explanation. Makes sense now. Just wasn't obvious from a cursory look at the code.

drlaw1558 · 2024-06-07T02:35:43Z

src/stcal/saturation/saturation.py

@@ -85,6 +84,20 @@ def flag_saturated_pixels(
            plane = data[ints, group, :, :]

            if read_pattern is not None:
+                if group == 2:


I don't think this code will 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.

flag_temp? I don't see that used anywhere in here. Where is it?

Sorry, copy-paste issue from my review of the jwst PR. Same general idea though- gdq for the third group isn't set until the code block after all the (if group==2) stuff, so it can never fulfill the criterion of identifying things where saturation was set in the third group.

drlaw1558 · 2024-06-07T02:37:20Z

src/stcal/saturation/saturation.py

+                    mask = data[ints, 0, ...] / np.mean(read_pattern[0]) * read_pattern[2][-1] < sat_thresh
+
+                    # Identify groups with suspiciously large values in the second group
+                    mask &= data[ints, 1, ...] > sat_thresh / len(read_pattern[1])


I think we also need something to grow this mask by n_pix_grow_sat using the adjacent_pixels 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.

@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?

drlaw1558 · 2024-06-07T20:35:59Z

src/stcal/saturation/saturation.py

+
+                    # now, flag any pixels that border saturated pixels
+                    if n_pix_grow_sat > 0:
+                        mask = adjacent_pixels(mask, saturated, n_pix_grow_sat)


I don't think we can call adjacent_pixels directly; it takes an array with dq flag values, and mask is an array of True/False.

yes, I thought I changed this but now it should be ok. @drlaw1558 can you please verify?

drlaw1558 · 2024-06-20T14:17:48Z

src/stcal/saturation/saturation.py

+                    mask &= np.where(gdq[ints, 2, :, :] & saturated, True, False)
+
+                    # Flag the 2nd group for the pixels passing that gauntlet
+                    gdq[ints, 1][mask] |= saturated


I think we need to use a temporary array here; if we set SATURATED for things passing the gauntlet above in the gdq array, and then grow the gdq array, then we'll be growing many pixels that we didn't intend to which didn't pass the gauntlet. I.e., perhaps define a zero-valued gdq_temp, set SATURATED for things that pass the gauntlet, grow gdq_temp, then add gdq_temp into gdq[ints,1, ...]

penaguerrero · 2024-07-12T14:37:06Z

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

adds a check for saturation bias in group 2 for nframes > 1.

562b19c

penaguerrero added the saturation label May 21, 2024

penaguerrero requested a review from a team as a code owner May 21, 2024 20:08

penaguerrero added 2 commits May 22, 2024 11:55

minor changes

07c91f1

fixed tests

bea1015

github-actions bot added the testing label May 22, 2024

fixed tests

ff920e8

kmacdonald-stsci approved these changes May 22, 2024

View reviewed changes

avoid breaking Roman code

c7c941b

hbushouse mentioned this pull request May 29, 2024

JP-3593: Saturation step should increase flagging for group 3 saturation spacetelescope/jwst#8499

Closed

8 tasks

removing previous code and adding modification to read_pattern code

b1e9519

penaguerrero changed the title ~~WIP - JP-3593: Saturation step should increase flagging for group 3 saturation~~ JP-3593: Saturation step should increase flagging for group 3 saturation Jun 3, 2024

Merge branch 'main' into jp3593

06078fe

stscijgbot-jp mentioned this pull request Jun 5, 2024

Saturation step should increase flagging for group 3 saturation spacetelescope/jwst#8413

Closed

hbushouse reviewed Jun 5, 2024

View reviewed changes

drlaw1558 reviewed Jun 7, 2024

View reviewed changes

fixing calling the right array and index accessing

cb7aa94

drlaw1558 reviewed Jun 7, 2024

View reviewed changes

penaguerrero and others added 5 commits June 17, 2024 11:28

Merge branch 'main' into jp3593

93836d7

addressing comments

1f0441f

addressing comments

665a114

Merge branch 'main' into jp3593

04eb3fd

Merge branch 'main' into jp3593

2d32245

drlaw1558 reviewed Jun 20, 2024

View reviewed changes

penaguerrero closed this Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JP-3593: Saturation step should increase flagging for group 3 saturation #259

JP-3593: Saturation step should increase flagging for group 3 saturation #259

penaguerrero commented May 21, 2024 •

edited by hbushouse

Loading

codecov bot commented May 22, 2024 •

edited

Loading

kmacdonald-stsci left a comment

braingram commented May 22, 2024

schlafly commented May 22, 2024

schlafly commented May 22, 2024

hbushouse left a comment

hbushouse Jun 5, 2024

schlafly commented Jun 6, 2024

hbushouse commented Jun 6, 2024

drlaw1558 Jun 7, 2024

hbushouse Jun 7, 2024

drlaw1558 Jun 7, 2024

drlaw1558 Jun 7, 2024

penaguerrero Jun 7, 2024

drlaw1558 Jun 7, 2024

penaguerrero Jun 17, 2024

drlaw1558 Jun 20, 2024

penaguerrero commented Jul 12, 2024

JP-3593: Saturation step should increase flagging for group 3 saturation #259

JP-3593: Saturation step should increase flagging for group 3 saturation #259

Conversation

penaguerrero commented May 21, 2024 • edited by hbushouse Loading

codecov bot commented May 22, 2024 • edited Loading

Codecov Report

kmacdonald-stsci left a comment

Choose a reason for hiding this comment

braingram commented May 22, 2024

schlafly commented May 22, 2024

schlafly commented May 22, 2024

hbushouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlafly commented Jun 6, 2024

hbushouse commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penaguerrero commented Jul 12, 2024

penaguerrero commented May 21, 2024 •

edited by hbushouse

Loading

codecov bot commented May 22, 2024 •

edited

Loading