JP-3677: Add maximum_shower_amplitude parameter to jump step #306
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #306 +/- ##
==========================================
- Coverage 86.38% 82.25% -4.13%
==========================================
Files 49 48 -1
Lines 8899 8897 -2
==========================================
- Hits 7687 7318 -369
- Misses 1212 1579 +367 ☔ View full report in Codecov by Sentry. |
|
Not sure why I can't request a review, but tagging @kmacdonald-stsci |
|
The romancal regression tests pass |
There was a problem hiding this comment.
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.
src/stcal/jump/jump.py
Outdated
| @@ -317,6 +320,7 @@ def detect_jumps( | |||
| ) | |||
| log.info("Total snowballs = %i", total_snowballs) | |||
| number_extended_events = total_snowballs | |||
|
|
|||
| if find_showers: | |||
There was a problem hiding this comment.
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.
src/stcal/jump/jump.py
Outdated
| # 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 |
There was a problem hiding this comment.
This could be combined into a single computation by setting invalid_flags = donotuse_flag | sat_flag | jump_flag.
src/stcal/jump/jump.py
Outdated
| # 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() |
There was a problem hiding this comment.
Add spaces after ,.
src/stcal/jump/jump.py
Outdated
| # 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 |
There was a problem hiding this comment.
Add spaces after ,.
src/stcal/jump/jump.py
Outdated
| # 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 |
There was a problem hiding this comment.
Add spaces after ,.
src/stcal/jump/jump.py
Outdated
| # 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]] |
There was a problem hiding this comment.
Add spaces after ,.
src/stcal/jump/jump.py
Outdated
| # 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 |
There was a problem hiding this comment.
This could be combined into a single computation by setting invalid_flags = donotuse_flag | sat_flag | jump_flag.
| @@ -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): | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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. |
|
Added one more minor change based on feedback from @mwregan2 to fix a prior bug with propagating flags to smoothed masks. |
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
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)"git+https://github.com/<fork>/stcal@<branch>")jwstregression testromancalregression testnews fragment change types...
changes/<PR#>.apichange.rst: change to public APIchanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.general.rst: infrastructure or miscellaneous change