spacetelescope / stcal

https://stcal.readthedocs.io/en/latest/
Other
10 stars 32 forks source link

JP-3734: Changed Default CI Test Algorithm to OLS_C #298

Closed kmacdonald-stsci closed 3 weeks ago

kmacdonald-stsci commented 3 weeks ago

Resolves JP-3734

Closes #

This PR addresses default algorithm testing for ramp fitting. The OLS_C algorithm is now the default algorithm for testing.

This PR also addresses a bug that resulted in incorrect attempts to do the CHARGELOSS recalculations when not needed, which would result in a crash when trying to dereference a NULL pointer for rd->orig_gdq.

The PR revealed another bug in the creation of the optional results product. The arrays were being created using PyArray_EMPTY, which didn't initialize memory, so unused elements in some of the optional results product arrays were junk. The creation function used now is PyArray_ZEROS, which initializes the array to zero.

Tasks

news fragment change types... - ``changes/.apichange.rst``: change to public API - ``changes/.bugfix.rst``: fixes an issue - ``changes/.general.rst``: infrastructure or miscellaneous change
codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 86.95652% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.80%. Comparing base (5f94030) to head (a83e20b). Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
tests/test_ramp_fitting.py 77.77% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #298 +/- ## ========================================== - Coverage 85.19% 81.80% -3.40% ========================================== Files 46 46 Lines 8804 8810 +6 ========================================== - Hits 7501 7207 -294 - Misses 1303 1603 +300 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kmacdonald-stsci commented 3 weeks ago

Regression test:

https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1769/

Edit: No failures for this test run.

kmacdonald-stsci commented 3 weeks ago

I recreated the orig_gdq error noted in

https://github.com/spacetelescope/jwst/issues/8842

using the main branch using the command

strun calwebb_detector1 jw01523003001_03102_00001_mirifulong_uncal.fits --steps.ramp_fit.maximum_cores=half

I verified this PR fixes this error.

tapastro commented 3 weeks ago

@schlafly I believe this PR touches code that Roman also uses, and it looks like Dave is OoO. Are you willing to review this?

schlafly commented 3 weeks ago

Thanks. We turned off the "ordinary" ramp fitting unit tests in romancal ~last month, and for our regression tests we use the Casertano+22 stuff, so I doubt this affects Roman. But I am running some regtests now.

schlafly commented 3 weeks ago

This ran fine on the Roman side, thanks! https://github.com/spacetelescope/RegressionTests/actions/runs/11238326482/job/31242982194