JP-3677: Add maximum_shower_amplitude parameter to jump step #306

drlaw1558 · 2024-10-15T21:48:04Z

This PR addresses JP-3677 by adding a maximum_shower_amplitude parameter to the MIRI cosmic ray showers code, and ensuring that any changes in the rate image due to shower flagging are below this amplitude.

See also corresponding jwst PR spacetelescope/jwst#8890

Tasks

update or add relevant tests
update relevant docstrings and / or docs/ page
Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
- write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
- run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
  - jwst regression test
  - romancal regression test

news fragment change types...

changes/<PR#>.apichange.rst: change to public API
changes/<PR#>.bugfix.rst: fixes an issue
changes/<PR#>.general.rst: infrastructure or miscellaneous change

codecov · 2024-10-15T21:51:21Z

Codecov Report

Attention: Patch coverage is 6.89655% with 27 lines in your changes missing coverage. Please review.

Project coverage is 29.56%. Comparing base (26836b8) to head (728b877).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
src/stcal/jump/jump.py	6.89%	27 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (26836b8) and HEAD (728b877). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (26836b8) HEAD (728b877)

3 2

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #306       +/-   ##
===========================================
- Coverage   86.68%   29.56%   -57.13%     
===========================================
  Files          49       37       -12     
  Lines        8937     8332      -605     
===========================================
- Hits         7747     2463     -5284     
- Misses       1190     5869     +4679

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

drlaw1558 · 2024-10-15T21:53:34Z

Not sure why I can't request a review, but tagging @kmacdonald-stsci

ddavis-stsci · 2024-10-16T14:44:01Z

The romancal regression tests pass
https://github.com/spacetelescope/RegressionTests/actions/runs/11365425316
so we're good with the changes

kmacdonald-stsci

There are portions of the code that are invoked unconditionally, but included in conditionals, which results in two parts of the code that need to be maintained, when there could be a single unconditional call to be maintained.

Also, there are some formatting issues.

kmacdonald-stsci · 2024-10-17T12:58:44Z

src/stcal/jump/jump.py

@@ -317,6 +320,7 @@ def detect_jumps(
            )
            log.info("Total snowballs = %i", total_snowballs)
            number_extended_events = total_snowballs
+
        if find_showers:


As noted in another PR, the find_showers portion of the code is always invoked in both the if and else portion of the conditional. Since it is always executed unconditionally, it should be moved outside the conditional. This applies to the expand_large_events portion of the code, too.

kmacdonald-stsci · 2024-10-17T13:00:56Z

src/stcal/jump/jump.py

+        # Approximate pre-shower rates
+        tempdata = indata[intg,:,:,:].copy()
+        # Ignore any groups flagged in the original gdq array
+        tempdata[ingdq[intg,:,:,:] & donotuse_flag != 0] = np.nan


This could be combined into a single computation by setting invalid_flags = donotuse_flag | sat_flag | jump_flag.

kmacdonald-stsci · 2024-10-17T13:01:30Z

src/stcal/jump/jump.py

+    # Ensure that flagging showers didn't change final fluxes by more than the allowed amount
+    for intg in range(nints):
+        # Approximate pre-shower rates
+        tempdata = indata[intg,:,:,:].copy()


Add spaces after ,.

kmacdonald-stsci · 2024-10-17T13:01:44Z

src/stcal/jump/jump.py

+        # Approximate pre-shower rates
+        tempdata = indata[intg,:,:,:].copy()
+        # Ignore any groups flagged in the original gdq array
+        tempdata[ingdq[intg,:,:,:] & donotuse_flag != 0] = np.nan


Add spaces after ,.

kmacdonald-stsci · 2024-10-17T13:02:16Z

src/stcal/jump/jump.py

+        # Approximate post-shower rates
+        tempdata = indata[intg,:,:,:].copy()
+        # Ignore any groups flagged in the shower gdq array
+        tempdata[gdq[intg,:,:,:] & donotuse_flag != 0] = np.nan


Add spaces after ,.

kmacdonald-stsci · 2024-10-17T13:02:31Z

src/stcal/jump/jump.py

+        # became NaN or changed by more than the amount reasonable for a real CR shower
+        diff = np.abs(image1 - image2)
+        indx = np.where((np.isfinite(diff) == False)|(diff > max_shower_amplitude))
+        gdq[intg,:,indx[0],indx[1]] = ingdq[intg,:,indx[0],indx[1]]


Add spaces after ,.

kmacdonald-stsci · 2024-10-17T13:03:03Z

src/stcal/jump/jump.py

+        # Approximate post-shower rates
+        tempdata = indata[intg,:,:,:].copy()
+        # Ignore any groups flagged in the shower gdq array
+        tempdata[gdq[intg,:,:,:] & donotuse_flag != 0] = np.nan


This could be combined into a single computation by setting invalid_flags = donotuse_flag | sat_flag | jump_flag.

kmacdonald-stsci · 2024-10-17T13:06:49Z

src/stcal/jump/jump.py

@@ -1111,6 +1123,39 @@ def find_faint_extended(
                num_grps_masked=num_grps_masked,
                max_extended_radius=max_extended_radius
            )
+
+    # Ensure that flagging showers didn't change final fluxes by more than the allowed amount
+    for intg in range(nints):


Is this loop needed? Is it possible to operate on the full data set, taking advantage of fast numpy looping, instead of slow explicit looping?

Here the worry was that calling .copy() on the entire array could be extremely memory intensive for cases where there were a large number of integrations. Looping over the integration is less efficient, but it looks like it still only takes 3 seconds even for a TSO case broken into 18 ints, 30 groups, and 1024x1032 pixels.

kmacdonald-stsci

LGTM

drlaw1558 · 2024-10-18T16:29:13Z

Updated to catch additional runtime warnings, and ensure the maximum amplitude is passed in the correct units so that it works for both FAST and SLOW mode data.

drlaw1558 · 2024-11-08T22:02:38Z

Added one more minor change based on feedback from @mwregan2 to fix a prior bug with propagating flags to smoothed masks.

tapastro

Change fragment needs renamed from 307 --> 306. Otherwise looks good, will approve pending fix.

tapastro · 2024-11-26T21:30:40Z

It looks like a primary result of the regresssion tests is a difference in how reference pixels are labeled - the test output shows the reference pixels now lack the DNU flag. Is this intended behavior?

drlaw1558 · 2024-11-27T21:00:52Z

@tapastro Just double checked this for both MIRI MRS and Imaging detectors, and that change is fine. Without running find_showers the reference pixels do not get flagged as DO_NOT_USE by the jump step, and so this change actually makes their treatment more consistent between whether or not the showers algorithm was run.

Add maximum_shower_amplitude parameter to jump step

45da1b3

drlaw1558 requested a review from a team as a code owner October 15, 2024 21:48

github-actions bot added the jump label Oct 15, 2024

drlaw1558 mentioned this pull request Oct 15, 2024

JP-3677: Add maximum_shower_amplitude parameter to jump step spacetelescope/jwst#8890

Merged

10 tasks

drlaw1558 marked this pull request as draft October 15, 2024 21:51

drlaw1558 marked this pull request as ready for review October 15, 2024 21:52

stscijgbot-jp mentioned this pull request Oct 15, 2024

CR showers code excessive flagging in MRS bright sources spacetelescope/jwst#8616

Open

drlaw1558 added 2 commits October 15, 2024 17:56

Add changelog

743c4e1

Change to nanmean, clean up unit test

0ab554a

github-actions bot added the testing label Oct 16, 2024

kmacdonald-stsci requested changes Oct 17, 2024

View reviewed changes

Code cleanup

c3e176f

kmacdonald-stsci approved these changes Oct 17, 2024

View reviewed changes

Catch additional warning, update comments

3f6f8d3

Propagate masked pixels

a0cd517

tapastro added 2 commits November 14, 2024 10:59

Merge branch 'main' into jp-3677

4133414

Merge branch 'main' into jp-3677

2f0ab96

tapastro requested changes Nov 18, 2024

View reviewed changes

tapastro mentioned this pull request Nov 19, 2024

update stcal requirement spacetelescope/jwst#8962

Closed

10 tasks

tapastro added 3 commits November 25, 2024 12:07

Merge branch 'main' into jp-3677

1900157

Rename 307.jump.rst to 306.jump.rst

40077fb

Rename 306.jump.rst to 306.apichange.rst

728b877

tapastro requested a review from schlafly December 2, 2024 20:36

schlafly approved these changes Dec 2, 2024

View reviewed changes

tapastro self-requested a review December 3, 2024 17:04

tapastro approved these changes Dec 3, 2024

View reviewed changes

tapastro merged commit 30dd769 into spacetelescope:main Dec 3, 2024
23 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JP-3677: Add maximum_shower_amplitude parameter to jump step #306

JP-3677: Add maximum_shower_amplitude parameter to jump step #306

drlaw1558 commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

drlaw1558 commented Oct 15, 2024

ddavis-stsci commented Oct 16, 2024

kmacdonald-stsci left a comment

kmacdonald-stsci Oct 17, 2024

drlaw1558 Oct 17, 2024

kmacdonald-stsci Oct 17, 2024

drlaw1558 Oct 17, 2024

kmacdonald-stsci Oct 17, 2024

drlaw1558 Oct 17, 2024

kmacdonald-stsci Oct 17, 2024

drlaw1558 Oct 17, 2024

kmacdonald-stsci Oct 17, 2024

drlaw1558 Oct 17, 2024

kmacdonald-stsci Oct 17, 2024

drlaw1558 Oct 17, 2024

kmacdonald-stsci Oct 17, 2024

drlaw1558 Oct 17, 2024

kmacdonald-stsci Oct 17, 2024

drlaw1558 Oct 17, 2024 •

edited

Loading

kmacdonald-stsci left a comment

drlaw1558 commented Oct 18, 2024

drlaw1558 commented Nov 8, 2024

tapastro left a comment

tapastro commented Nov 26, 2024

drlaw1558 commented Nov 27, 2024

JP-3677: Add maximum_shower_amplitude parameter to jump step #306

JP-3677: Add maximum_shower_amplitude parameter to jump step #306

Conversation

drlaw1558 commented Oct 15, 2024 • edited Loading

Tasks

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

drlaw1558 commented Oct 15, 2024

ddavis-stsci commented Oct 16, 2024

kmacdonald-stsci left a 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

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

Choose a reason for hiding this comment

drlaw1558 Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

kmacdonald-stsci left a comment

Choose a reason for hiding this comment

drlaw1558 commented Oct 18, 2024

drlaw1558 commented Nov 8, 2024

tapastro left a comment

Choose a reason for hiding this comment

tapastro commented Nov 26, 2024

drlaw1558 commented Nov 27, 2024

drlaw1558 commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

drlaw1558 Oct 17, 2024 •

edited

Loading