Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Sep 19, 2024
Merged
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ ramp_fitting
- Moved the read noise variance recalculation for CHARGELOSS flagging to the C
implementation, when the algorithm is ``OLS_C``. [#8697, spacetelescope/stcal#278]

- Updated the flow of the detector 1 pipeline when selecting the ``LIKELY`` algorithm
for ramp fitting. The ramps must contain a minimum number of groups (4).[#8631]

resample
--------

Expand Down
5 changes: 5 additions & 0 deletions docs/jwst/ramp_fitting/description.rst
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ JWST-specific features are described later within this document.
Upon successful completion of this step, the status keyword S_RAMP will be set
to "COMPLETE".

There is a new algorithm available for testing in ramp fitting, which is the
likelihood algorithm. It is selected by setting ``--ramp_fitting.algorithm=LIKELY``.
The details are in the ``stcal`` documentation at
:ref:`Likelihood Algorithm Details <stcal:likelihood_algo>`.
kmacdonald-stsci marked this conversation as resolved.
Show resolved Hide resolved

Multiprocessing
---------------
This step has the option of running in multiprocessing mode. In that mode it will
Expand Down
20 changes: 15 additions & 5 deletions jwst/ramp_fitting/ramp_fit_step.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
from stcal.ramp_fitting import ramp_fit
from stcal.ramp_fitting import utils

from stcal.ramp_fitting.likely_fit import LIKELY_MIN_NGROUPS

from stcal.ramp_fitting.utils import LARGE_VARIANCE
from stcal.ramp_fitting.utils import LARGE_VARIANCE_THRESHOLD

Expand Down Expand Up @@ -385,7 +387,7 @@ class RampFitStep(Step):
class_alias = "ramp_fit"

spec = """
algorithm = option('OLS', 'OLS_C', default='OLS_C') # 'OLS' and 'OLS_C' use the same underlying algorithm, but OLS_C is implemented in C
algorithm = option('OLS', 'OLS_C', 'LIKELY', default='OLS_C') # 'OLS' and 'OLS_C' use the same underlying algorithm, but OLS_C is implemented in C
int_name = string(default='')
save_opt = boolean(default=False) # Save optional output
opt_name = string(default='')
Expand Down Expand Up @@ -414,8 +416,16 @@ def process(self, input):
readnoise_filename = self.get_reference_file(input_model, 'readnoise')
gain_filename = self.get_reference_file(input_model, 'gain')

log.info('Using READNOISE reference file: %s', readnoise_filename)
log.info('Using GAIN reference file: %s', gain_filename)
ngroups = input_model.data.shape[1]
if self.algorithm.upper() == "LIKELY" and ngroups < LIKELY_MIN_NGROUPS:
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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.algorithm = "OLS_C"

log.info(f"Using READNOISE reference file: {readnoise_filename}")
log.info(f"Using GAIN reference file: {gain_filename}")

with datamodels.ReadnoiseModel(readnoise_filename) as readnoise_model, \
datamodels.GainModel(gain_filename) as gain_model:
Expand All @@ -432,8 +442,8 @@ def process(self, input):
readnoise_2d, gain_2d = get_reference_file_subarrays(
input_model, readnoise_model, gain_model, frames_per_group)

log.info('Using algorithm = %s' % self.algorithm)
log.info('Using weighting = %s' % self.weighting)
log.info(f"Using algorithm = {self.algorithm}")
log.info(f"Using weighting = {self.weighting}")

buffsize = ramp_fit.BUFSIZE
if self.algorithm == "GLS":
Expand Down