Enable users to run and re-run one step of Calibrations #1877

profxj · 2024-12-04T17:15:32Z

A script and related to code to run to one step of the calibrations and stop.

This is mainly useful to re-run a bad calibration step.

Docs included and a test in the DevSuite in an accompanying PR.

Part of the motivation here is to enable a future Dashboard.

debora-pe

Hi @profxj . Thanks for doing this. I have a few comments (minor), but I also tested the code and works great.
We should run the test.

debora-pe · 2024-12-13T20:45:26Z

pypeit/pypeit.py

+
+                self.calib_one(grp_frames, self.det)
+
+#                # Instantiate Calibrations class


The commented lines can be removed

debora-pe · 2024-12-13T20:51:18Z

pypeit/calibrations.py

+                'remake' -- Force the frame to be remade.
+                'reload' -- Reload the frame if it exists.
+                None -- Load the existing frame if it exists and reuse_calibs=True
+


It seems like the force if loop is missing here (line 499)

debora-pe · 2024-12-13T20:54:52Z

pypeit/calibrations.py

@@ -793,7 +865,8 @@ def get_flats(self):
        calib_key = illum_calib_key if pixel_calib_key is None else pixel_calib_key
        setup = illum_setup if pixel_setup is None else pixel_setup
        calib_id = illum_calib_id if pixel_calib_id is None else pixel_calib_id
-        if cal_file.exists() and self.reuse_calibs:
+
+        if cal_file.exists() and self.reuse_calibs and not force == 'remake':


Is the force=reload option missing here?

Oof. I forgot how complicated the flat file is...

debora-pe · 2024-12-13T21:01:55Z

pypeit/calibrations.py

+            if stop_at_step is not None and step == stop_at_step:
+                force = 'remake'
+            else:
+                force = None


It doesn't seem force=reload is actually an option. Should we remove it from the if loop in each step?

debora-pe · 2024-12-13T21:08:25Z

pypeit/scripts/run_to_calibstep.py

+        parser.add_argument('pypeit_file', type=str,
+                            help='PypeIt reduction file (must have .pypeit extension)')
+        parser.add_argument('frame', type=str, help='Raw science frame to reduce as listed in your PypeIt file, e.g. b28.fits.gz')
+        parser.add_argument('step', type=str, help='Calibration step to perform')


It would be good to provide the list of possible steps here. Or, we could add another parameter, e.g. --list_steps, which will list all the default steps for that Calibrations class.

Good suggestion. I would define a class attribute:

valid_steps = ['align', 'arc', 'bias', 'bpm', 'dark', 'flats', 'scattlight', 'slits', 'tiltimg', 'tilts', 'wv_calib']

or a class method:

@classmethod def valid_steps(cls): steps = [method.replace('get_', '') for method in dir(cls) \ if callable(getattr(cls, method)) and method.startswith('get_')] steps.remove('instance') steps.remove('association') return steps

Then here, you would just do:

from pypeit.calibrations import Calibrations parser.add_argument('step', type=str, help=f'Calibration step to perform. Valid steps are {', '.join(Calibrations.valid_steps)}')

with or without the () for valid_steps, depending on the implementation you choose.

kbwestfall

A few relatively minor comments from me.

kbwestfall · 2024-12-18T20:01:20Z

pypeit/calibrations.py

+        elif force == 'reload' and not cal_file.exists():
+            self.success = False
+            return
+        elif force == 'reload' or (self.reuse_calibs and cal_file.exists()): 


I recommend abstracting this a bit. I'd add this member function:

def process_load_selection(self, frame, cal_file, force): """ Process how pypeit should use any pre-existing calibration files. If loading is requested but the calibration file (``cal_file``) does not exist, ``self.success`` is set to False, and None is returned. Args: frame (:obj:`dict`): A dictionary with two elements: ``type`` is the string defining the frame type and ``class`` is the pypeit class used to load the pre-existing calibration file. cal_file (:obj:`str`, `Path`_): Path to the calibration file. force (:obj:`str`): Defines how to treat a pre-existing calibration file. Must be one of the following options: - ``'remake'``: Force the calibration be remade. - ``'reload'``: Reload the frame if it exists. - ``None``: Load the existing frame if it exists and ``self.reuse_calibs=True``. Returns: :obj:`object`: Either the loaded calibration object or None. """ if force not in [None, 'remake', 'reload']: msgs.error(f'`force` keyword must be None, remake, or reload, not {force}') if force == 'remake': return None _cal_file = Path(cal_file).absolute() if force == 'reload' and not _cal_file.exists(): msgs.warn(f"{_cal_file} does not exist; cannot reload " f"{frame['class'].__name__} calibration.") self.success = False return None if force == 'reload' or (self.reuse_calibs and _cal_file.exists()): return = frame['class'].from_file(_cal_file, chk_version=self.chk_version)

And then the lines within each get_* function would replace this block with:

self.?? = self.process_load_selection(frame, cal_file, force) if not self.success or self.?? is not None: return self.??

where ?? is msarc in this function.

kbwestfall · 2024-12-18T20:02:04Z

pypeit/calibrations.py

+                'remake' -- Force the frame to be remade.
+                'reload' -- Reload the frame if it exists.
+                None -- Load the existing frame if it exists and reuse_calibs=True
+


kbwestfall · 2024-12-18T20:02:46Z

pypeit/calibrations.py

        """
        Load or generate the bad pixel mask.

        This is primarily a wrapper for
        :func:`~pypeit.spectrographs.spectrograph.Spectrograph.bpm`.

+        Args:
+            force (:obj:`str`, optional):


Thanks for adding this. Also add a description of frame?

kbwestfall · 2024-12-18T20:05:42Z

pypeit/calibrations.py

@@ -793,7 +865,8 @@ def get_flats(self):
        calib_key = illum_calib_key if pixel_calib_key is None else pixel_calib_key
        setup = illum_setup if pixel_setup is None else pixel_setup
        calib_id = illum_calib_id if pixel_calib_id is None else pixel_calib_id
-        if cal_file.exists() and self.reuse_calibs:
+
+        if cal_file.exists() and self.reuse_calibs and not force == 'remake':


Oof. I forgot how complicated the flat file is...

kbwestfall · 2024-12-18T20:07:06Z

pypeit/calibrations.py

@@ -1099,7 +1189,7 @@ def get_wv_calib(self):
                               'calibration! Please generate a new one with a 2d fit.')

            # Return
-            if self.par['wavelengths']['redo_slits'] is None:
+            if (self.par['wavelengths']['redo_slits'] is None) or self.try_reload_only:


Is try_reload_only defined?

kbwestfall · 2024-12-18T20:14:01Z

pypeit/scripts/run_to_calibstep.py

+        """
+        Return the name of the executable.
+        """
+        return 'pypeit_run_to_calibstep'


You don't need to define name. The default in the base class results in the same thing.

kbwestfall · 2024-12-18T20:16:10Z

pypeit/scripts/run_to_calibstep.py

+        import argparse
+
+        parser = super().get_parser(description='Run PypeIt to a single calibration step for an input frame',
+                                    width=width, formatter=argparse.RawDescriptionHelpFormatter)


I think you're better off with the default formatter; the reason this is used in run_pypeit.py is so that the parser can handle the usage string.

kbwestfall · 2024-12-18T20:28:35Z

pypeit/scripts/run_to_calibstep.py

+        parser.add_argument('pypeit_file', type=str,
+                            help='PypeIt reduction file (must have .pypeit extension)')
+        parser.add_argument('frame', type=str, help='Raw science frame to reduce as listed in your PypeIt file, e.g. b28.fits.gz')
+        parser.add_argument('step', type=str, help='Calibration step to perform')


Good suggestion. I would define a class attribute:

valid_steps = ['align', 'arc', 'bias', 'bpm', 'dark', 'flats', 'scattlight', 'slits', 'tiltimg', 'tilts', 'wv_calib']

or a class method:

@classmethod def valid_steps(cls): steps = [method.replace('get_', '') for method in dir(cls) \ if callable(getattr(cls, method)) and method.startswith('get_')] steps.remove('instance') steps.remove('association') return steps

Then here, you would just do:

from pypeit.calibrations import Calibrations parser.add_argument('step', type=str, help=f'Calibration step to perform. Valid steps are {', '.join(Calibrations.valid_steps)}')

with or without the () for valid_steps, depending on the implementation you choose.

kbwestfall · 2024-12-18T20:34:53Z

pypeit/scripts/run_to_calibstep.py

+        splitnm = os.path.splitext(args.pypeit_file)
+        if splitnm[1] != '.pypeit':
+            msgs.error('Input file must have a .pypeit extension!')
+        logname = splitnm[0] + ".log"


I recommend that we avoid using os.path for path manipulation in new code. I would change this to:

from pathlib import Path _pypeit_file = Path(args.pypeit_file).absolute() if _pypeit_file.suffix != '.pypeit': msgs.error(f'Input file {_pypeit_file} must have a .pypeit extension!') logname = _pypeit_file.parent / f'{_pypeit_file.stem}.log'

kbwestfall · 2024-12-18T20:39:41Z

pypeit/scripts/run_to_calibstep.py

+                                    width=width, formatter=argparse.RawDescriptionHelpFormatter)
+        parser.add_argument('pypeit_file', type=str,
+                            help='PypeIt reduction file (must have .pypeit extension)')
+        parser.add_argument('frame', type=str, help='Raw science frame to reduce as listed in your PypeIt file, e.g. b28.fits.gz')


Would it make sense to allow the users to provide a calibration group or a science file? I understand why you're using the science file (it defines the fitstbl row and therefore the calibration group), but can we just provide the calibration group directly? It feels odd that we're requiring users provide a specific science frame for this script.

Related, can we also allow the user to specify the detector or mosaic?

profxj added 17 commits December 2, 2024 06:34

digging in

3971f14

wip

63f8a95

successful model

572c382

modify force

ea890d7

scrip

16fefdd

todo

f0353f8

steps

4b8cba7

mo

4b75168

other steps

e09e30d

stop at step

afc7469

running on all steps

1f830bf

doc and script

1f21b31

setup and det

fae34a6

polish

d066534

allowed steps

28ddcc0

changes

2a433e5

more docs

cc64636

profxj requested review from kbwestfall and debora-pe December 4, 2024 17:15

profxj mentioned this pull request Dec 4, 2024

Tests for pypeit_run_to_calibstep pypeit/PypeIt-development-suite#341

Open

debora-pe requested changes Dec 13, 2024

View reviewed changes

kbwestfall requested changes Dec 18, 2024

View reviewed changes

kbwestfall added 2 commits December 18, 2024 12:41

Merge branch 'develop' into state_and_steps

1debdb2

Merge branch 'develop' into state_and_steps

c5657c9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable users to run and re-run one step of Calibrations #1877

Enable users to run and re-run one step of Calibrations #1877

profxj commented Dec 4, 2024

debora-pe left a comment

debora-pe Dec 13, 2024

debora-pe Dec 13, 2024

kbwestfall Dec 18, 2024

debora-pe Dec 13, 2024

kbwestfall Dec 18, 2024

debora-pe Dec 13, 2024

debora-pe Dec 13, 2024

kbwestfall Dec 18, 2024

kbwestfall left a comment

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024

kbwestfall Dec 18, 2024


		self.calib_one(grp_frames, self.det)

		# # Instantiate Calibrations class

Enable users to run and re-run one step of Calibrations #1877

Are you sure you want to change the base?

Enable users to run and re-run one step of Calibrations #1877

Conversation

profxj commented Dec 4, 2024

debora-pe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbwestfall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment