Skip to content

JP-3835: Modify the Saturation Step and Jump Step to Use Only Saturation for Snowball Detection - #576

Open
kmacdonald-stsci wants to merge 9 commits into
spacetelescope:mainfrom
kmacdonald-stsci:jp_3835_jump_02
Open

JP-3835: Modify the Saturation Step and Jump Step to Use Only Saturation for Snowball Detection#576
kmacdonald-stsci wants to merge 9 commits into
spacetelescope:mainfrom
kmacdonald-stsci:jp_3835_jump_02

Conversation

@kmacdonald-stsci

Copy link
Copy Markdown
Collaborator

Resolves JP-3835

This PR addresses the use of the DO_NOT_USE flag during the saturation step for better results in the snowball detection algorithm in the jump step. The use of the DO_NOT_USE flag in the saturation step has been changed to use the SATURATED flag and the snowball detection algorithm has been updated to properly identify snowballs with only the use of the SATURATED flag.

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 changelog readme for instructions)
    • if your change breaks existing functionality, also add a changes/<PR#>.breaking.rst news fragment
  • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.97%. Comparing base (a0bee58) to head (47c5777).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #576      +/-   ##
==========================================
+ Coverage   91.92%   91.97%   +0.05%     
==========================================
  Files          63       64       +1     
  Lines        8925     9002      +77     
==========================================
+ Hits         8204     8280      +76     
- Misses        721      722       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@drlaw1558

Copy link
Copy Markdown
Contributor

Copying over some comments and figures from an offline exchange for posterity:

Thanks, I just tested this out on jw01345001001_10201_00001_nrca3_uncal and I'm seeing mixed results. In the attached, 'Original' is the current pipeline, 'SimpleChange' is what you get if you just change dnu to saturated in saturation.py, and 'Aug26PR' is what you get with this PR changing both saturation.py and jump.py

case1.png attached shows the snowball around pixel (x,y) = (270, 874). The simple change doesn't pick up the extended snowball halo properly, while the PR does. Looks good.

case2.png attached shows the snowball around pixel (x,y) = (1551, 787). Neither the simple change nor the PR picks up the extended snowball halo as well as the current pipeline. However, both do a better job in the core of not flagging too much.

I'm not sure I understand why this PR recovers current performance for one of these snowballs and not the other. Any thoughts?

Similar results for other NIRCam data from the same program.

Testing on a NIRSpec image (jw02123001001_13101_00001_nrs2_uncal) I'm seeing very minor differences (good), likely because of the larger ngroups.

case1 case2

@melanieclarke

melanieclarke commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

I'm not sure I understand why this PR recovers current performance for one of these snowballs and not the other. Any thoughts?

I think this is because of JP-4282: the other snowball saturates in the first group and snowball detection currently starts iterating on the second group. Adding a similar change to #566 on top of this branch fixes the ring for this snowball, showing a much closer result to the current main branch version.

group0_fix

@drlaw1558

Copy link
Copy Markdown
Contributor

Ah, thanks @melanieclarke , I'll take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants