Skip to content

JP-4318: Updating Ramp Fitting Tests for New STCAL Slope Computation.#550

Open
kmacdonald-stsci wants to merge 11 commits into
spacetelescope:mainfrom
kmacdonald-stsci:jp_4318_cr_rfit
Open

JP-4318: Updating Ramp Fitting Tests for New STCAL Slope Computation.#550
kmacdonald-stsci wants to merge 11 commits into
spacetelescope:mainfrom
kmacdonald-stsci:jp_4318_cr_rfit

Conversation

@kmacdonald-stsci

Copy link
Copy Markdown
Collaborator

Resolves JP-4318

This PR addresses using the median rate of a pixel, which is computed across all integrations of a pixel, for the computation of the rate and rateint slopes, using the Poisson variance as part of the weights. This should not happen, since this has the effect of later integrations affecting the slope of earlier integrations. To prevent this the weighting during the computation of rate and rateint slopes use only the read noise variance, rather than a combination of both Poisson and read noise variances.

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

@kmacdonald-stsci kmacdonald-stsci requested a review from a team as a code owner June 4, 2026 17:22
@github-actions github-actions Bot added documentation Improvements or additions to documentation ramp_fitting testing labels Jun 4, 2026
@kmacdonald-stsci

Copy link
Copy Markdown
Collaborator Author

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.78%. Comparing base (a04ec58) to head (54156c9).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
- Coverage   91.80%   91.78%   -0.03%     
==========================================
  Files          63       63              
  Lines        8855     8857       +2     
==========================================
  Hits         8129     8129              
- Misses        726      728       +2     

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

Comment thread docs/stcal/ramp_fitting/description.rst Outdated
@drlaw1558

Copy link
Copy Markdown
Contributor

In order to properly reflect the weights being used in the optional output data products I think we also need to change:

Lines 3191-3192 from:

tmp = 1. / seg->var_e;
seg->weight = tmp * tmp;

to

tmp = 1. / seg->var_r;
seg->weight = tmp

Lines 3284-3287 from:

tmp = (seg->var_p + seg->var_r);
wt = 1. / tmp;
wt *= wt;
seg->weight = wt;

to

tmp = seg->var_r;
wt = 1. / tmp;
seg->weight = wt;

Lines 3442-3443 from:

seg->weight = 1. / seg->var_e;
seg->weight *= seg->weight;

to

seg->weight = 1. / seg->var_r;

Comment thread src/stcal/ramp_fitting/src/slope_fitter.c Outdated

@drlaw1558 drlaw1558 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.

Looks good to me in local testing now; thanks @kmacdonald-stsci !

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

Looks like there are a number of unit tests that need updating still.

Comment thread docs/stcal/ramp_fitting/description.rst Outdated
Comment thread docs/stcal/ramp_fitting/description.rst Outdated
@kmacdonald-stsci

kmacdonald-stsci commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

Looks like there are a number of unit tests that need updating still.

All the "cases", which tests optional results, were failing due to the change in the weighting computation. They have all been updated and passing now.

@tapastro tapastro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this PR is good to go, but the paired JWST PR still has some questions. I'll hold off on merging until that one is squared away.

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