JP-3669: Updating the C Extension to do CHARGELOSS Read Noise Recalculations

[kmacdonald-stsci]

Resolves JP-3669

Closes #8601

This PR addresses the read noise variance recalculation based on the CHARGELOSS flag. This recalculation was in the JWST ramp fit step code, but that came with a performance hit and was unnecessarily complicated. The recalculation has been implemented in the C extension code, simplifying the ramp fitting, as well as improving performance.

There is a corresponding JWST PR that removes the CHARGELOSS recalculation from the step code, as well as removes the corresponding test. spacetelescope/jwst#8678

The regression tests pass, except for one test. The test failure has been confirmed to be incorrect. The regression test truth file for this test needs to be updated:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1623/

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]

Codecov Report

Attention: Patch coverage is 72.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 84.63%. Comparing base (3b0f596) to head (d85f859).
Report is 38 commits behind head on main.

Files with missing lines	Patch %	Lines
src/stcal/ramp_fitting/ramp_fit.py	10.00%	9 Missing ⚠️
src/stcal/ramp_fitting/ramp_fit_class.py	47.05%	9 Missing ⚠️
src/stcal/ramp_fitting/ols_fit.py	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #275      +/-   ##
==========================================
+ Coverage   84.38%   84.63%   +0.25%     
==========================================
  Files          41       41              
  Lines        7557     7628      +71     
==========================================
+ Hits         6377     6456      +79     
+ Misses       1180     1172       -8     

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

[melanieclarke]

I'm not great at C, but this looks plausible to me. Just a couple comments on the Python parts and some typos.

[drlaw1558]

Linked to spacetelescope/jwst#8697

Looks good to me! I've successfully tested that:

1. Runtime improves as expected in test case jw02079004003_03101_00001_nis. In the old version it takes about 2.5 minutes to run the relevant piece of the RN recalculation, while with this PR it takes about 2.5 seconds.

2. RN variances stay as expected in test case jw01510001001_02108_00001_nis using both the OLS and OLS_C algorithms.

3. Checked that the SCI and variance arrays did not change for other instruments that don't use charge migration.

[melanieclarke]

All my comments were addressed, thanks!

I think this should be good to go, once the changelog format is fixed and conflicts are resolved. It might be a good idea to run the regression tests one last time before merging.

The corresponding JWST PR can be merged as soon as this one is in.

[tapastro]

This needs an update to move the changelog entry to a towncrier fragment, then we'll get this merged ASAP in preparation for an stcal build.

[tapastro]

Tests are showing a number of failures. It looks like the gls_fit tests have out of date dqflags, and the downstream tests show the same failures wrt. level 2 pipelines across PRs. We can get this PR in, but we should clean up these failures. There isn't much value gained by having tests if we ignore them due to repeated failures.

[kmacdonald-stsci]

Tests are showing a number of failures. It looks like the gls_fit tests have out of date dqflags, and the downstream tests show the same failures wrt. level 2 pipelines across PRs. We can get this PR in, but we should clean up these failures. There isn't much value gained by having tests if we ignore them due to repeated failures.

I added the CHARGELOSS flag keyword to the GLS flags for testing. They now pass.

[braingram]

The romancal test failures:
https://github.com/spacetelescope/stcal/actions/runs/10744216957/job/29800655839?pr=275#step:10:1360
are due to roman_datamodels not having a CHARGELOSS dqflag.

@schlafly are there plans to support this in in romancal or alternatively can ols be dropped?

[schlafly]

Is there an option to turn on/off this behavior? For Roman we don't currently have this flag but we could add it; it does seem to me that we will have charge migration issues. On the other hand, this code isn't about flagging or addressing the charge migration per se. My understanding is that the "old" behavior gives a more accurate estimation of the read noise contribution to the variance of these pixels, but that we've decided that we don't want that more accurate estimation because it leads to high flux pixels being weighted downward in resample for IVM weighting. I might have approached this via something like:
spacetelescope/jwst#7563
where in resample one replaces the weights of saturated (or now also chargeloss'd) pixels with their unaffected neighbors, reasoning that one doesn't "really" care about the read noise variance for these pixels, where the poisson noise is dominant.

[braingram]

Is CHARGELOSS only supported by OLS_C and not OLS? If so, I think (and I tested this locally) passing in algorithm to this method and only setting flags_chargeloss if algorithm == OLS_C should fix the romancal tests.

[kmacdonald-stsci]

The CHARGELOSS flag is used when selecting either OLS_C or OLS. Currently, the use of the CHARGELOSS flag is handled in the JWST step code. Unfortunately, there is a bug in the segmentation computation (that occurs very rarely on an edge case) and with a computation hit.

When running ramp fitting, there is a message that the code returned from the STCAL code, but there can be a noticeable lag between that and the completion of the ramp fitting step. To fix the bug and to improve computational performance, I moved the read noise variance re-calculation from the JWST step code into the C-extension. This required some accounting changes in ramp_fit_class.py and ramp_fit.py to get the proper information to the C-extension.

[nden]

I got confirmation from RTB that they do not intend to use OLS_C or OLS for Roman.
If necessary, changes related to Roman will be done later. This can be merged for JWST.

[tapastro]

Happy to merge this ASAP, but it looks like one of the new unit tests is failing:

FAILED tests/test_ramp_fitting.py::test_cext_chargeloss - assert np.float32(0.000556237) != np.float32(0.000556237)

[kmacdonald-stsci]

Happy to merge this ASAP, but it looks like one of the new unit tests is failing:

FAILED tests/test_ramp_fitting.py::test_cext_chargeloss - assert np.float32(0.000556237) != np.float32(0.000556237)

I"ll check it out.

[kmacdonald-stsci]

Happy to merge this ASAP, but it looks like one of the new unit tests is failing:

FAILED tests/test_ramp_fitting.py::test_cext_chargeloss - assert np.float32(0.000556237) != np.float32(0.000556237)

It's fixed.

[tapastro]

LGTM - merging this momentarily.