JP-3502: Extend snowball flags to next integration #238

mwregan2 · 2024-01-05T19:38:06Z

This PR addresses the problem that the saturated core of snowballs shows up in the next integration with an excess count rate that takes a long time to decay. The solution is to mark as jump a number of groups passed in from JWST Jump.

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-01-05T19:40:16Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (5cdca94) 85.98% compared to head (8f3dcb4) 85.90%.

Files	Patch %	Lines
tests/test_jump.py	50.00%	4 Missing ⚠️
src/stcal/jump/jump.py	85.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
- Coverage   85.98%   85.90%   -0.08%     
==========================================
  Files          35       35              
  Lines        6542     6557      +15     
==========================================
+ Hits         5625     5633       +8     
- Misses        917      924       +7

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

kmacdonald-stsci · 2024-01-05T20:12:50Z

src/stcal/jump/jump.py

@@ -54,6 +54,8 @@ def detect_jumps(
    minimum_groups=3,
    minimum_sigclip_groups=100,
    only_use_ints=True,
+    mask_persist_grps_next_int=True,
+    persist_grps_flagged=25,


This function now has roughly 30 arguments. Would it be better served by using a class to contain the parameters, rather than continue to add parameters to this function?

I agree that it has a lot of parameters but users need to be able to tweak the parameters. Is there any cost to the large number of parameters?

The cost is readability and maintainability. Grouping various parameters into a class or classes makes the code easier to read, as well as easier to maintain, especially when adding a parameter that needs to be passed to subroutines. If you simply pass parameters, the function signature needs to be changed for all functions that need that parameter. If you use classes, those classes will be passed to the subroutines, so when you add a parameter to a class, no subroutine signature needs to be edited, since the parameter is contained in the class.

While it is true that passing objects instead of primitives has advantages there are also disadvantages. APIs with only primitive parameters are easier to understand where the primitives are coming from. At the extreme, a function that has a single class as input in a black box. Someone browsing the code has more work to do than with primitive parameters.
Also, If we want to consolidate the parameters passed to detect_jump.py, I feel that the change should be in a separate PR. That would give us a working master to compare against.
What do you think?

src/stcal/jump/jump.py

kmacdonald-stsci · 2024-01-05T20:19:13Z

src/stcal/jump/jump.py

+                -1,
+            )
+            persist_ellipse = persist_image[:, :, 2]
+            persist_saty, persist_satx = np.where(persist_ellipse == 22)


What is special about 22 here? A comment or declaring a descriptive variable with the value 22 would be good here.

I updated the comments in this section. Let me know, if you think it needs more details.

I still don't know why you use 22 instead of say 21 or 23.

No real reason that I can remember. There needs to be a number that I can use np.where to find the pixels that are affected. I'm sure there's a more elegant way to do it.

I think it would be fine to make a comment saying this number could change and that this number was determined though trial and error and could be changed if needed.

The value of 22 is arbitrary. It just needs to be something that can be tested for to create the mask of pixels within the ellipse.

CHANGES.rst

src/stcal/jump/jump.py

took suggestion Co-authored-by: Howard Bushouse <[email protected]>

fix spacing Co-authored-by: Howard Bushouse <[email protected]>

fuxed Co-authored-by: Howard Bushouse <[email protected]>

resolving comments

…gan2/stcal into extend_snowball_to_next_int

kmacdonald-stsci · 2024-01-29T15:06:37Z

src/stcal/jump/jump.py

+                -1,
+            )
+            persist_ellipse = persist_image[:, :, 2]
+            persist_saty, persist_satx = np.where(persist_ellipse == 22)


I still don't know why you use 22 instead of say 21 or 23.

kmacdonald-stsci · 2024-01-29T15:13:46Z

src/stcal/jump/jump.py

@@ -54,6 +54,8 @@ def detect_jumps(
    minimum_groups=3,
    minimum_sigclip_groups=100,
    only_use_ints=True,
+    mask_persist_grps_next_int=True,
+    persist_grps_flagged=25,


The cost is readability and maintainability. Grouping various parameters into a class or classes makes the code easier to read, as well as easier to maintain, especially when adding a parameter that needs to be passed to subroutines. If you simply pass parameters, the function signature needs to be changed for all functions that need that parameter. If you use classes, those classes will be passed to the subroutines, so when you add a parameter to a class, no subroutine signature needs to be edited, since the parameter is contained in the class.

hbushouse · 2024-01-31T17:43:01Z

Lots of CI failures that appear to be due to API changes in function calls.

hbushouse

Given that the only outstanding (unresolved) comments are related to stylistic issues, and not actual problems with the code, I'm going to approve in order to get this moving.

hbushouse · 2024-02-15T01:16:32Z

@mwregan2 Doesn't this need a corresponding change in the jump_step.py module in the jwst package, in order to add the 2 new step arguments that've been implemented here and pass them to the detect_jumps function? I don't see an existing PR against jwst that contains such changes.

mwregan2 · 2024-02-15T16:03:04Z

I’ve got the branch ready. I do the PR today From: Howard Bushouse ***@***.***> Reply-To: spacetelescope/stcal ***@***.***> Date: Wednesday, February 14, 2024 at 8:16 PM To: spacetelescope/stcal ***@***.***> Cc: Michael Regan ***@***.***>, Mention ***@***.***> Subject: Re: [spacetelescope/stcal] JP-3502: Extend snowball flags to next integration (PR #238) @mwregan2<https://urldefense.com/v3/__https:/github.com/mwregan2__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!1NXfDtfYo8HSoZ6v0E7RmpmHHIKvFdjtrsWx0pLRSHeSgrttPx_cnJlh4CvFGoGwNYB0Aav6ilvQIzGqcsMHQ_E$> Doesn't this need a corresponding change in the jump_step.py module in the jwst package, in order to add the 2 new step arguments that've been implemented here and pass them to the detect_jumps function? I don't see an existing PR against jwst that contains such changes. — Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/238*issuecomment-1945210344__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!1NXfDtfYo8HSoZ6v0E7RmpmHHIKvFdjtrsWx0pLRSHeSgrttPx_cnJlh4CvFGoGwNYB0Aav6ilvQIzGq1z4B65k$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AF6KJLQFEY6PRSEKWXYFFKDYTVOXZAVCNFSM6AAAAABBO36D5CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBVGIYTAMZUGQ__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!1NXfDtfYo8HSoZ6v0E7RmpmHHIKvFdjtrsWx0pLRSHeSgrttPx_cnJlh4CvFGoGwNYB0Aav6ilvQIzGq_y9CDjw$>. You are receiving this because you were mentioned.Message ID: ***@***.***>

mwregan2 added 13 commits January 3, 2024 12:45

Update jump.py

6fbeb56

add fits

85df783

Update jump.py

6da60a8

Update grps flagging

79128ae

fixes

33f63b4

Update jump.py

1a28914

Update jump.py

bc20f4d

Update jump.py

605f845

Update jump.py

44cb702

cleanup

b36bdb5

remove fits

fbb0d5b

Update test_jump.py

cc23831

Update jump.py

c9cacac

mwregan2 requested a review from a team as a code owner January 5, 2024 19:38

github-actions bot added testing jump labels Jan 5, 2024

Update CHANGES.rst

4bb6455

kmacdonald-stsci approved these changes Jan 5, 2024

View reviewed changes

hbushouse changed the title ~~Extend snowball to next int~~ JP-3502: Extend snowball flags to next integration Jan 25, 2024

hbushouse reviewed Jan 25, 2024

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

src/stcal/jump/jump.py Outdated Show resolved Hide resolved

src/stcal/jump/jump.py Outdated Show resolved Hide resolved

mwregan2 and others added 5 commits January 26, 2024 15:22

Update CHANGES.rst

57d8fb3

took suggestion Co-authored-by: Howard Bushouse <[email protected]>

Update src/stcal/jump/jump.py

1a3319a

fix spacing Co-authored-by: Howard Bushouse <[email protected]>

Update src/stcal/jump/jump.py

46f96e4

fuxed Co-authored-by: Howard Bushouse <[email protected]>

Update jump.py

b75dd06

resolving comments

Merge branch 'extend_snowball_to_next_int' of https://github.com/mwre…

9d5df58

…gan2/stcal into extend_snowball_to_next_int

kmacdonald-stsci approved these changes Jan 29, 2024

View reviewed changes

mwregan2 and others added 2 commits February 1, 2024 10:02

Update jump.py

0ba61ac

Merge branch 'main' into extend_snowball_to_next_int

8f3dcb4

hbushouse approved these changes Feb 15, 2024

View reviewed changes

hbushouse merged commit 44f50f2 into spacetelescope:main Feb 15, 2024
23 of 25 checks passed

stscijgbot-jp mentioned this pull request Feb 15, 2024

Snowballs cause persistence in the Integration after the Snowball spacetelescope/jwst#8174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JP-3502: Extend snowball flags to next integration #238

JP-3502: Extend snowball flags to next integration #238

mwregan2 commented Jan 5, 2024 •

edited by hbushouse

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

kmacdonald-stsci Jan 5, 2024

mwregan2 Jan 26, 2024

kmacdonald-stsci Jan 29, 2024

mwregan2 Feb 13, 2024

kmacdonald-stsci Jan 5, 2024

mwregan2 Jan 26, 2024

kmacdonald-stsci Jan 29, 2024

mwregan2 Feb 1, 2024

kmacdonald-stsci Feb 1, 2024 •

edited

Loading

mwregan2 Feb 13, 2024

kmacdonald-stsci Jan 29, 2024

kmacdonald-stsci Jan 29, 2024

hbushouse commented Jan 31, 2024

hbushouse left a comment

hbushouse commented Feb 15, 2024

mwregan2 commented Feb 15, 2024 via email

JP-3502: Extend snowball flags to next integration #238

JP-3502: Extend snowball flags to next integration #238

Conversation

mwregan2 commented Jan 5, 2024 • edited by hbushouse Loading

codecov bot commented Jan 5, 2024 • edited Loading

Codecov Report

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

kmacdonald-stsci Feb 1, 2024 • edited Loading

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

hbushouse commented Jan 31, 2024

hbushouse left a comment

Choose a reason for hiding this comment

hbushouse commented Feb 15, 2024

mwregan2 commented Feb 15, 2024 via email

mwregan2 commented Jan 5, 2024 •

edited by hbushouse

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

kmacdonald-stsci Feb 1, 2024 •

edited

Loading