JP-3967: SATURATION flag for partially saturated pixels should be propagated to the _rate DQ

[pllim]

Resolves JP-3967

This PR addresses spacetelescope/jwst#8124 by reverting a bit of #125

xref

• JP-3121: initial Code to Implement C-Extensions for Ramp Fit #156

Target build: 12.3

Notes from Ken MacDonald

There are several past PRs related to your ticket:

• JP-3014: This Sets Invalid Slope Data to NaN in the Rate and Rateints products. #133
• JP-3014: This Sets Invalid Slope Data to NaN in the Rate and Rateints products. #131
• JP-2988: Set saturation only if fully saturated. #125
• JP-2669: Fixing the NaN and DO_NOT_USE bug. #112

In particular, the SAT flag for an integration was set only if the entire integration was set. This has a couple of consequences. When SAT is set, so is DNU. This invalidates the data, making their values NaN.

Unfortunately, due to the "LIKELY" algorithm being done in Python, this is done in two parts of the code:

• utils.py for the “LIKELY” and
• slope_fitter.c for the “OLS” algorithm.

Below are links to the relevant parts of the C extension.

The initial set up for SAT flagging is done here (done on intial read):

stcal/src/stcal/ramp_fitting/src/slope_fitter.c

Line 1891 in b326357

pr->stats[integ].cnt_sat++;

Notice there is a cnt_sat variable that gets updated. This makes it easy to fine fully saturated ramps.

The setting of the integration level SAT is done here (done during processing):

stcal/src/stcal/ramp_fitting/src/slope_fitter.c

Line 1295 in b326357

if ( (!chargeloss) && (groupdq[0] & rd->sat)) {

Here is the seeting of pedestal data based on the integration level saturation:

stcal/src/stcal/ramp_fitting/src/slope_fitter.c

Line 1989 in b326357

if (pr->rateints[integ].dq & rd->sat) {

Invalid data is defined as DNU | SAT:

stcal/src/stcal/ramp_fitting/src/slope_fitter.c

Line 2148 in b326357

rd->invalid = rd->dnu | rd->sat;

If a ramp is flagged as SAT, set its slope to NaN:

stcal/src/stcal/ramp_fitting/src/slope_fitter.c

Line 2807 in b326357

if (pr->rateints[integ].dq & rd->sat) {

If all ramps are flagged as SAT, flag exposure level pixel as SAT:

stcal/src/stcal/ramp_fitting/src/slope_fitter.c

Line 2820 in b326357

if (rd->nints == sat_cnt) {

If the entire ramp is flagged as SAT, flag the integration as SAT (done during non-chargeloss processing):

stcal/src/stcal/ramp_fitting/src/slope_fitter.c

Line 3005 in b326357

pr->rateints[integ].dq |= rd->sat;

(At this point, Melanie Clarke and Tyler Pauly stressed that this PR should only change DQ array, not SCI nor ERR.)

When you updated the utils.py file for DQ flagging, the test file that should have been updated was test_ramp_fitting_likely_fit.py. The changes you made in utils.py do not affect the tests in test_ramp_fitting.py, which is where you made all the test updates.

The tests for ramp fitting are like this:

• test_ramp_fitting_cas22.py is for Roman, so no updates we do should affect this file.
• The LIKELY algorithm affects tests in test_ramp_fitting_likely_fit.py.
• The OLS algorithm (the C-extension) will affect the other two ramp fitting test modules: test_ramp_fitting.py and test_ramp_fitting_cases.py

TODO

• Discuss with Ken on the state of this PR. Why did Ken's commit not fix the failing tests, given the tests have been updated to values expected on successful reversion? How to proceed?
• See if test_ramp_fitting_cases.py also needs updating. Why have the changes so far not affecting it? What is it actually testing?
• Change slope_fitter.c more so test_ramp_fitting.py and test_ramp_fitting_cases.py pass.
• Change test_ramp_fitting_likely_fit.py with theoretical expected result for successful reversion, then fix utils.py.
• Verify expected results for LIKELY algorithm added to test_ramp_fitting.py

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 below for change types)
  • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>") stcal@git+https://github.com/pllim/stcal.git@life-of-a-sat-grid
    • jwst regression test https://github.com/spacetelescope/RegressionTests/actions/runs/19578858266 and https://github.com/spacetelescope/RegressionTests/actions/runs/19901284665 and https://github.com/spacetelescope/RegressionTests/actions/runs/20248199486 and https://github.com/spacetelescope/RegressionTests/actions/runs/20289156418
    • romancal regression test https://github.com/spacetelescope/RegressionTests/actions/runs/19578882303 and https://github.com/spacetelescope/RegressionTests/actions/runs/20248238781 and https://github.com/spacetelescope/RegressionTests/actions/runs/20289178794

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.02%. Comparing base (17e1705) to head (63391e8).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #421      +/-   ##
==========================================
+ Coverage   89.89%   90.02%   +0.13%     
==========================================
  Files          65       65              
  Lines        9983    10114     +131     
==========================================
+ Hits         8974     9105     +131     
  Misses       1009     1009              

☔ View full report in Codecov by Sentry.
📢 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.

[kmacdonald-stsci]

I don't mind. We probably should have worked to get this in first, then reformatted, but 🤷🏻‍♂️.

[pllim]

@kmacdonald-stsci I could use some advice on whether the results for LIKELY added here make sense or I messed up something in utils.py. Thanks!

[kmacdonald-stsci]

@pllim That appears correct. Further, many of the tests of the LIKELY algorithm are checked against the OLS algorithm as shown at the link below. These tests would fail if the flagging was different.

stcal/tests/test_ramp_fitting_likely_fit.py

Line 144 in 1272f00

algo = "OLS"

[pllim]

There is one failure on romancal that looks related (test_liklihood_rampfit: romancal.regtest.test_ramp_fit_likelihood) though the diff is hard for me to understand.

https://github.com/spacetelescope/RegressionTests/actions/runs/19578882303/attempts/1#summary-56071572672

FAILED romancal/regtest/test_ramp_fit_likelihood.py::test_liklihood_rampfit - AssertionError: 
  Diff report for:
      result file: r0000101001001001001_0001_wfi01_f158_like_rampfit.asdf
          model type: ImageModel
      truth file: truth/r0000101001001001001_0001_wfi01_f158_like_rampfit.asdf
          model type: ImageModel
  
  {'arrays_differ': {"root['roman']['dq']": {'abs_diff': np.uint64(2018634628180),
                                             'n_diffs': np.int64(313),
                                             'worst_abs_diff': {'index': (np.int64(401),
                                                                          np.int64(522)),
                                                                'value': np.uint32(4294967294)},
                                             'worst_fractional_diff': {'index': (np.int64(0),
                                                                                 np.int64(23)),
                                                                       'value': np.float64(1.0)}}}}

[pllim]

52 failures in jwst RT! https://github.com/spacetelescope/RegressionTests/actions/runs/19578858266/attempts/1#summary-56071629269

[schlafly]

I'm happy to okify the romancal regtest after this is merged, but please tell me when that happens so I can be ready.

We should probably do the same for romancal / Casertano+22, but that's currently handled within romancal so I can tackle it separately.

(maybe just to say---yes, I agree that those romancal regtest failures look expected given this change)

[pllim]

I went down the rabbit hole of trying to get flagging (more) consistent between OLS and LIKELY in test_ramp_fitting.py and found that I have to move this up to make it happen. Is there a reason why this is only done for OLS but not LIKELY before?

[melanieclarke]

There might be. @kmacdonald-stsci @t-brandt - do you know?

[kmacdonald-stsci]

The test_zeroframe was developed years ago, before the LIKELY algorithm was added. When the LIKELY algorithm was added, it did not use the ZEROFRAME. Usage of the ZEROFRAME wasn't added to the LIKELY algorithm till a few months ago.

Further, the test_ramp_fitting.py test module was designed to test the ramp fitting default algorithm, not the LIKELY algorithm, which is tested in the test_ramp_fitting_likely.py test module. There is an expectation these algorithms would give different results, so the same tests would yield different results.

[pllim]

Does that mean I have to undo this change here?

[kmacdonald-stsci]

If you have been using this module to test the LIKELY algorithm, then yes. Use test_ramp_fitting_likely.py to test that algorithm.

[melanieclarke]

@kmacdonald-stsci - the question here was whether it's appropriate to use suppress_one_group_ramps with the likelihood algorithm as well as OLS. The current behavior is to use it only for OLS.

I think this is a side question for this PR, though, so I suggest we leave it as is and file a ticket for follow up later.

[kmacdonald-stsci]

I don't really understand the use cases for the LIKELY algorithm. I don't know the use cases, nor why someone would use it. For the OLS case, Mike Regan requested ramps with only one good group be suppressed. I don't know if the reason for that extends to the LIKELY algorithm.

[pllim]

OK I opened https://jira.stsci.edu/browse/JP-4208 (spacetelescope/jwst#10089). We can discuss there. Thanks!

[pllim]

@melanieclarke , do I have to also change dq_compress_sect here to go with your previous suggestion for dq_compress_final?

Also unsure if I have to touch L109 above to change it to set_if_total_ramp(pixeldq_sect, gdq_sect, dnu, dnu).

Given LIKELY can behave so differently from OLS, I am not sure what to expect.

[melanieclarke]

No, this one looks right as is.

At L109, it is setting DNU for the integration if the whole ramp is saturated, which is still correct behavior.

After that, it is now setting a SAT flag for the integration if there was saturation anywhere in the ramp, which is also correct.