spacetelescope / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
561 stars 167 forks source link

JP-3658: JWST Accommodation of the Ramp Fitting Likelihood Algorithm #8631

Closed kmacdonald-stsci closed 1 week ago

kmacdonald-stsci commented 2 months ago

Resolves JP-3658

Closes #

This PR addresses the use of the likelihood algorithm with ramp fitting. This updates the spec for the arguments of ramp fitting to allow for LIKELY as an algorithm option. Also, the detector1 pipeline is modified to do some special handling needed when using the likelihood algorithm. In particular, there data needs at least 4 groups per integration and, since the likelihood algorithm does its own jump detection, the normal pipeline jump detection needs to be skipped when using the likelihood algorithm.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 61.75%. Comparing base (5783cfb) to head (9c55a0a). Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
jwst/ramp_fitting/ramp_fit_step.py 77.77% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8631 +/- ## ========================================== - Coverage 61.75% 61.75% -0.01% ========================================== Files 377 377 Lines 38750 38755 +5 ========================================== + Hits 23931 23934 +3 - Misses 14819 14821 +2 ```

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

melanieclarke commented 3 weeks ago

Looking better, thanks! It looks like we still need documentation updates for the ramp fit step, though.

melanieclarke commented 3 weeks ago

Testing on some real data with ngroups=3, it looks like it warns and switches to OLS_C correctly when the step is called from the detector1 pipeline. When ramp_fit is called directly, though, it reports Using algorithm = LIKELY, but then the step unexpectedly succeeds -- it looks like it's quietly defaulting to OLS (python) instead. It should warn and default to OLS_C.

Sorry for the late realization, but since we are no longer turning off jump detection by default, perhaps this check should actually just be moved from the detector1 pipeline to the ramp fit step instead, so we can handle both calling conditions in the same place.

melanieclarke commented 1 week ago

@kmacdonald-stsci - coming back to this one, now that the stcal PR is close to ready. Can we move the check for ngroups into the ramp_fit step and add some likelihood documentation?

kmacdonald-stsci commented 1 week ago

@kmacdonald-stsci - coming back to this one, now that the stcal PR is close to ready. Can we move the check for ngroups into the ramp_fit step and add some likelihood documentation?

If you want the NGROUPS check moved to the step code, then it can probably just be moved to STCAL, so the only change for this PR will be the addition of the LIKELY option to the algorithm parameter.

melanieclarke commented 1 week ago

If you want the NGROUPS check moved to the step code, then it can probably just be moved to STCAL, so the only change for this PR will be the addition of the LIKELY option to the algorithm parameter.

It looks to me like the ramp_fit_step interface in jwst reports the algorithm it's going to use before it calls the stcal code, so it would be nice to have the check there, before that happens. We will also need the documentation updates on the jwst side.

tapastro commented 1 week ago

EDIT: Scratch that, pin suggestion being taken care of in separate PR. I think I can take it from here.