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

[kmacdonald-stsci]

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)

• added entry in CHANGES.rst within the relevant release section
• updated or added relevant tests
• updated relevant documentation
• added relevant milestone
• added relevant label(s)
• ran regression tests, post a link to the Jenkins job below.
  How to run regression tests on a PR
• All comments are resolved
• Make sure the JIRA ticket is resolved properly

[codecov]

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 ⚠️

Additional details and impacted files

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

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

[melanieclarke]

This needs to wait until the stcal PR is merged:
spacetelescope/stcal#278

In the meantime, a few comments on the structure and log messages here. Also, we'll need to update the documentation here, to match the changes in stcal and add the new option for the algorithm argument.

[melanieclarke]

A few more small tidy-up comments

[melanieclarke]

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

[melanieclarke]

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]

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

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

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.

[melanieclarke]

This message is long - let's split it up into two messages and make them warnings. Also, the f needs to go at the front of all the substrings to get the variables in there properly.

Suggested change

	log.info(f"When selecting the LIKELY ramp fitting algorithm the"
	" ngroups needs to be a minimum of {likely_fit.LIKELY_MIN_NGROUPS},"
	" but ngroups = {ngroups}. Due to this, the ramp fitting algorithm"
	" is being changed to OLS_C")
	log.warning(f"When selecting the LIKELY ramp fitting algorithm the"
	f" ngroups needs to be a minimum of {LIKELY_MIN_NGROUPS},"
	f" but ngroups = {ngroups}.")
	log.warning("Due to this, the ramp fitting algorithm"
	" is being changed to OLS_C.")

[melanieclarke]

Thanks for fixing the variable reference. Can you please also add the f at the beginning of the substrings, so the variables are printed properly? Also, split into two messages and make it a warning instead of an info.

[tapastro]

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