Skip to content

Propagate flags applied during likelihood ramp fitting into the input groupdq#558

Merged
schlafly merged 4 commits into
spacetelescope:mainfrom
schlafly:update-groupdq
Jul 6, 2026
Merged

Propagate flags applied during likelihood ramp fitting into the input groupdq#558
schlafly merged 4 commits into
spacetelescope:mainfrom
schlafly:update-groupdq

Conversation

@schlafly

@schlafly schlafly commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Currently the likelihood ramp fitting can identify and flag jumps and snowballs in the ramps, but it does not have a mechanism to propagate the 3D locations of those jumps back to the caller. It instead propagates only the 2D information. I here change the code to update the groupdq array in place so that the caller sees the updated groupdq following ramp fitting.

I also update docs and tests and 'fix' some tests that depended on the groupdq not being updated.

I think we should always do this and have written the code that way. It does change the behavior, though, so I'd also be open to introducing a new keyword argument that turns on this behavior, which would make this change fully backward compatible.

The underlying motivation for providing this information back to the caller is that some folks want to look for asteroids in Roman data, and want to identify those as a set of adjacent jumps that move from resultant to resultant as the asteroid moves within a single image.

Regression tests are clean.
Roman (doesn't use the groupdq image after ramp fitting): https://github.com/spacetelescope/RegressionTests/actions/runs/28527846891/job/84569105552
Webb (I don't actually know to what extent they exercise the likelihood-based ramp fitting): https://github.com/spacetelescope/RegressionTests/actions/runs/28527858422/job/84569142314

I think the main open question is whether we'd like to instead add some kind of flag that controls this behavior rather than having it always on. But this design seems good to me and it makes the likelihood-based jump detection provide similar information to the other jump detection, which is good.

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>")

@github-actions github-actions Bot added documentation Improvements or additions to documentation ramp_fitting testing labels Jul 1, 2026
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.90%. Comparing base (f84085b) to head (1c54a3e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #558      +/-   ##
==========================================
+ Coverage   91.88%   91.90%   +0.01%     
==========================================
  Files          63       63              
  Lines        8913     8926      +13     
==========================================
+ Hits         8190     8203      +13     
  Misses        723      723              

☔ 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.

@schlafly
schlafly marked this pull request as ready for review July 1, 2026 17:36
@schlafly
schlafly requested a review from a team as a code owner July 1, 2026 17:36
@zacharyburnett

zacharyburnett commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

what would be the impact of not making this toggleable with a keyword? Could we interpret existing 2D groupdq from jump / snow as extending the entire 3D axis (indeterminate where they are)?

@schlafly

schlafly commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

@zacharyburnett , yes, the current 2D pixeldq is basically np.bitwise_or(groupdq, axis=resultants). So that interpretation is right. It's just not what is needed for finding moving objects, which are remarkable in that you get clusters of adjacent jumps in different resultants. The main impact of lacking a toggle is that users of the step will start getting updated groupdq. So things like a handful of tests that reuse the groupdq expecting to get the same answer will break. I don't think scientifically we often want to do things like call the same step multiple times with the same groupdq, so this doesn't bother me, but it's a change of behavior.

@melanieclarke , Brett mentioned to me that you have been making sure that Webb steps don't change the input models, which this would break, but that maybe you only send copies to stcal and so this has no impact---apologies if I got that wrong. I'm happy insofar as this has no Webb regtest implications, but maybe that background makes you more leery of this type of change?

@melanieclarke

Copy link
Copy Markdown
Contributor

@melanieclarke , Brett mentioned to me that you have been making sure that Webb steps don't change the input models, which this would break, but that maybe you only send copies to stcal and so this has no impact---apologies if I got that wrong. I'm happy insofar as this has no Webb regtest implications, but maybe that background makes you more leery of this type of change?

Thanks for asking! The work we've been doing is to copy the input data up front when the step starts up, if needed, then make sure processing happens in-place as much as possible thereafter to avoid additional copies. The change here to update the DQ in place with the new flags looks fine to me, from that perspective.

@melanieclarke melanieclarke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have no impact for JWST -- the input groupdq is not used after calling ramp fit. We do exercise the likelihood algorithm for some data sets in our regression tests, so the clean regtest results are relevant!

Small typo in the change log. Otherwise, this looks fine to me for JWST.

Comment thread changes/558.ramp_fitting.rst Outdated
schlafly and others added 2 commits July 2, 2026 15:39
@schlafly

schlafly commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks everyone, merging!

@schlafly
schlafly merged commit 9ee7450 into spacetelescope:main Jul 6, 2026
26 checks passed
@schlafly
schlafly deleted the update-groupdq branch July 6, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation ramp_fitting testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants