Merge branch 'combine_refactor' of https://github.com/pypeit/PypeIt i…

…nto lris_jun24

pypeit/calibrations.py

@@ -4,6 +4,7 @@
 .. include common links, assuming primary doc root is up one directory
 .. include:: ../include/links.rst
 """
+import os
 from pathlib import Path
 from datetime import datetime
 from copy import deepcopy
@@ -209,6 +210,46 @@ def __init__(self, fitstbl, par, spectrograph, caldir, qadir=None,
         self.success = False
         self.failed_step = None
 
+    def check_calibrations(self, file_list, check_lamps=True):
+        """
+        Check if the input calibration files are consistent with each other.
+        This step is usually needed when combining calibration frames of a given type.
+        This routine currently only prints out warning messages if the calibration files are not consistent.
+
+        Note: The exposure times are currently checked in the combine step, so they are not checked here.
+
+        Parameters
+        ----------
+        file_list : list
+            List of calibration files to check
+        check_lamps : bool, optional
+            Check if the lamp status is the same for all the files. Default is True.
+        """
+
+        lampstat = [None] * len(file_list)
+        # Loop on the files
+        for ii, ifile in enumerate(file_list):
+            # Save the lamp status
+            headarr = deepcopy(self.spectrograph.get_headarr(ifile))
+            lampstat[ii] = self.spectrograph.get_lamps_status(headarr)
+
+        # Check that the lamps being combined are all the same
+        if check_lamps:
+            if not lampstat[1:] == lampstat[:-1]:
+                msgs.warn("The following files contain different lamp status")
+                # Get the longest strings
+                maxlen = max([len("Filename")] + [len(os.path.split(x)[1]) for x in file_list])
+                maxlmp = max([len("Lamp status")] + [len(x) for x in lampstat])
+                strout = "{0:" + str(maxlen) + "}  {1:s}"
+                # Print the messages
+                print(msgs.indent() + '-' * maxlen + "  " + '-' * maxlmp)
+                print(msgs.indent() + strout.format("Filename", "Lamp status"))
+                print(msgs.indent() + '-' * maxlen + "  " + '-' * maxlmp)
+                for ff, file in enumerate(file_list):
+                    print(msgs.indent()
+                          + strout.format(os.path.split(file)[1], " ".join(lampstat[ff].split("_"))))
+                print(msgs.indent() + '-' * maxlen + "  " + '-' * maxlmp)
+
     def find_calibrations(self, frametype, frameclass):
         """
         Find calibration files and identifiers.
@@ -334,6 +375,9 @@ def get_arc(self):
         # Reset the BPM
         self.get_bpm(frame=raw_files[0])
 
+        # Perform a check on the files
+        self.check_calibrations(raw_files)
+
         # Otherwise, create the processed file.
         msgs.info(f'Preparing a {frame["class"].calib_type} calibration frame.')
         self.msarc = buildimage.buildimage_fromlist(self.spectrograph, self.det,
@@ -377,6 +421,9 @@ def get_tiltimg(self):
         # Reset the BPM
         self.get_bpm(frame=raw_files[0])
 
+        # Perform a check on the files
+        self.check_calibrations(raw_files)
+
         # Otherwise, create the processed file.
         msgs.info(f'Preparing a {frame["class"].calib_type} calibration frame.')
         self.mstilt = buildimage.buildimage_fromlist(self.spectrograph, self.det,
@@ -426,6 +473,9 @@ def get_align(self):
         # Reset the BPM
         self.get_bpm(frame=raw_files[0])
 
+        # Perform a check on the files
+        self.check_calibrations(raw_files)
+
         # Otherwise, create the processed file.
         msgs.info(f'Preparing a {frame["class"].calib_type} calibration frame.')
         msalign = buildimage.buildimage_fromlist(self.spectrograph, self.det,
@@ -473,6 +523,9 @@ def get_bias(self):
             self.msbias = frame['class'].from_file(cal_file, chk_version=self.chk_version)
             return self.msbias
 
+        # Perform a check on the files
+        self.check_calibrations(raw_files)
+
         # Otherwise, create the processed file.
         msgs.info(f'Preparing a {frame["class"].calib_type} calibration frame.')
         self.msbias = buildimage.buildimage_fromlist(self.spectrograph, self.det,
@@ -523,6 +576,9 @@ def get_dark(self):
         # there any reason why creation of the bpm should come after the dark,
         # or can we change the order?
 
+        # Perform a check on the files
+        self.check_calibrations(raw_files)
+
         # Otherwise, create the processed file.
         self.msdark = buildimage.buildimage_fromlist(self.spectrograph, self.det,
                                                      self.par['darkframe'], raw_files,
@@ -597,6 +653,9 @@ def get_scattlight(self):
         # Reset the BPM
         self.get_bpm(frame=raw_scattlight_files[0])
 
+        # Perform a check on the files
+        self.check_calibrations(raw_scattlight_files)
+
         binning = self.fitstbl[scatt_idx[0]]['binning']
         dispname = self.fitstbl[scatt_idx[0]]['dispname']
         scattlightImage = buildimage.buildimage_fromlist(self.spectrograph, self.det,
@@ -725,6 +784,10 @@ def get_flats(self):
         if len(raw_pixel_files) > 0:
             # Reset the BPM
             self.get_bpm(frame=raw_pixel_files[0])
+
+            # Perform a check on the files
+            self.check_calibrations(raw_pixel_files)
+
             msgs.info('Creating pixel-flat calibration frame using files: ')
             for f in raw_pixel_files:
                 msgs.prindent(f'{Path(f).name}')
@@ -737,6 +800,10 @@ def get_flats(self):
             if len(raw_lampoff_files) > 0:
                 # Reset the BPM
                 self.get_bpm(frame=raw_lampoff_files[0])
+
+                # Perform a check on the files
+                self.check_calibrations(raw_lampoff_files)
+
                 msgs.info('Subtracting lamp off flats using files: ')
                 for f in raw_lampoff_files:
                     msgs.prindent(f'{Path(f).name}')
@@ -763,6 +830,10 @@ def get_flats(self):
         if not pix_is_illum and len(raw_illum_files) > 0:
             # Reset the BPM
             self.get_bpm(frame=raw_illum_files[0])
+
+            # Perform a check on the files
+            self.check_calibrations(raw_illum_files)
+
             msgs.info('Creating slit-illumination flat calibration frame using files: ')
             for f in raw_illum_files:
                 msgs.prindent(f'{Path(f).name}')
@@ -775,6 +846,10 @@ def get_flats(self):
                 for f in raw_lampoff_files:
                     msgs.prindent(f'{Path(f).name}')
                 if lampoff_flat is None:
+                    # Perform a check on the files
+                    self.check_calibrations(raw_lampoff_files)
+
+                    # Build the image
                     lampoff_flat = buildimage.buildimage_fromlist(self.spectrograph, self.det,
                                                                   self.par['lampoffflatsframe'],
                                                                   raw_lampoff_files,
@@ -889,6 +964,9 @@ def get_slits(self):
         # Reset the BPM
         self.get_bpm(frame=raw_trace_files[0])
 
+        # Perform a check on the files
+        self.check_calibrations(raw_trace_files)
+
         traceImage = buildimage.buildimage_fromlist(self.spectrograph, self.det,
                                                     self.par['traceframe'], raw_trace_files,
                                                     bias=self.msbias, bpm=self.msbpm,
@@ -903,6 +981,9 @@ def get_slits(self):
             # Reset the BPM
             self.get_bpm(frame=raw_trace_files[0])
 
+            # Perform a check on the files
+            self.check_calibrations(raw_lampoff_files)
+
             lampoff_flat = buildimage.buildimage_fromlist(self.spectrograph, self.det,
                                                           self.par['lampoffflatsframe'],
                                                           raw_lampoff_files, dark=self.msdark,

pypeit/images/buildimage.py

@@ -263,7 +263,7 @@ def buildimage_fromlist(spectrograph, det, frame_par, file_list, bias=None, bpm=
             bpm=bpm, dark=dark, flatimages=flatimages, slits=slits, mosaic=mosaic))
 
     # Do it
-    combineImage = combineimage.CombineImage(rawImage_list, spectrograph, frame_par['process'])
+    combineImage = combineimage.CombineImage(rawImage_list, frame_par['process'])
     pypeitImage = combineImage.run(maxiters=maxiters, ignore_saturation=ignore_saturation)
     # Return class type, if returning any of the frame_image_classes
     cls = frame_image_classes[frame_par['frametype']] \

pypeit/images/combineimage.py

@@ -4,8 +4,6 @@
 .. include:: ../include/links.rst
 """
 
-import os
-
 from IPython import embed
 
 import numpy as np
@@ -18,6 +16,7 @@
 from pypeit.images import pypeitimage
 from pypeit.images import imagebitmask
 
+
 class CombineImage:
     """
     Process and combine detector images. 
@@ -26,14 +25,10 @@ class CombineImage:
         rawImages (:obj:`list`, :class:`~pypeit.images.pypeitimage.PypeItImage):  
             Either a single :class:`~pypeit.images.pypeitimage.PypeItImage` object or a list of one or more 
             of these objects to be combined into a an image.
-        spectrograph (:class:`~pypeit.spectrographs.spectrograph.Spectrograph`):
-            Spectrograph used to take the data.            
         par (:class:`~pypeit.par.pypeitpar.ProcessImagesPar`):
             Parameters that dictate the processing of the images.
 
     Attributes:
-        spectrograph (:class:`~pypeit.spectrographs.spectrograph.Spectrograph`):
-            Spectrograph used to take the data.
         det (:obj:`int`, :obj:`tuple`):
             The 1-indexed detector number(s) to process.
         par (:class:`~pypeit.par.pypeitpar.ProcessImagesPar`):
@@ -43,11 +38,10 @@ class CombineImage:
             to be combined.             
 
     """
-    def __init__(self, rawImages, spectrograph, par):
+    def __init__(self, rawImages, par):
         if not isinstance(par, pypeitpar.ProcessImagesPar):
             msgs.error('Provided ParSet for must be type ProcessImagesPar.')
         self.rawImages = list(rawImages) if hasattr(rawImages, '__len__') else [rawImages]
-        self.spectrograph = spectrograph
         self.par = par  # This musts be named this way as it is frequently a child
 
         # NOTE: nimgs is a property method.  Defining rawImages above must come
@@ -165,13 +159,8 @@ def run(self, ignore_saturation=False, maxiters=5):
                 rn2img_stack = np.zeros(shape, dtype=float)
                 basev_stack = np.zeros(shape, dtype=float)
                 gpm_stack = np.zeros(shape, dtype=bool)
-                lampstat = [None]*self.nimgs
                 exptime = np.zeros(self.nimgs, dtype=float)
 
-            # Save the lamp status
-            # TODO: As far as I can tell, this is the *only* place rawheadlist
-            # is used.  Is there a way we can get this from fitstbl instead?
-            lampstat[kk] = self.spectrograph.get_lamps_status(rawImage.rawheadlist)
             # Save the exposure time to check if it's consistent for all images.
             exptime[kk] = rawImage.exptime
             # Processed image
@@ -195,25 +184,8 @@ def run(self, ignore_saturation=False, maxiters=5):
             gpm_stack[kk] = rawImage.select_flag(invert=True)
             file_list.append(rawImage.filename)
 
-        # TODO JFH: This should not be here, but rather should be done in the Calibrations class. Putting in calibration specific
-        # lamp checking in an image combining class is not ideal. 
-        # Check that the lamps being combined are all the same:
-        if not lampstat[1:] == lampstat[:-1]:
-            msgs.warn("The following files contain different lamp status")
-            # Get the longest strings
-            maxlen = max([len("Filename")]+[len(os.path.split(x)[1]) for x in self.files])
-            maxlmp = max([len("Lamp status")]+[len(x) for x in lampstat])
-            strout = "{0:" + str(maxlen) + "}  {1:s}"
-            # Print the messages
-            print(msgs.indent() + '-'*maxlen + "  " + '-'*maxlmp)
-            print(msgs.indent() + strout.format("Filename", "Lamp status"))
-            print(msgs.indent() + '-'*maxlen + "  " + '-'*maxlmp)
-            for ff, file in enumerate(self.files):
-                print(msgs.indent()
-                      + strout.format(os.path.split(file)[1], " ".join(lampstat[ff].split("_"))))
-            print(msgs.indent() + '-'*maxlen + "  " + '-'*maxlmp)
-
-        # Do a similar check for exptime
+        # Check that all exposure times are consistent
+        # TODO: JFH suggests that we move this to calibrations.check_calibrations
         if np.any(np.absolute(np.diff(exptime)) > 0):
             # TODO: This should likely throw an error instead!
             msgs.warn('Exposure time is not consistent for all images being combined!  '
@@ -275,7 +247,7 @@ def run(self, ignore_saturation=False, maxiters=5):
                                        # NOTE: The detector is needed here so
                                        # that we can get the dark current later.
                                        detector=rawImage.detector,
-                                       PYP_SPEC=self.spectrograph.name,
+                                       PYP_SPEC=rawImage.PYP_SPEC,
                                        units='e-' if self.par['apply_gain'] else 'ADU',
                                        exptime=comb_texp, noise_floor=self.par['noise_floor'],
                                        shot_noise=self.par['shot_noise'])

pypeit/images/pypeitimage.py

@@ -91,6 +91,9 @@ class PypeItImage(datamodel.DataContainer):
             :class:`~pypeit.images.rawimage.RawImage.process`.
     """
 
+    # TODO These docs are confusing. The __init__ method needs to be documented just as it is for
+    # every other class that we have written in PypeIt, i.e. the arguments all need to be documented. They are not
+    # documented here and instead we have the odd Args documentation above. 
     version = '1.3.0'
     """Datamodel version number"""
 
@@ -161,6 +164,7 @@ def from_pypeitimage(cls, pypeitImage):
         # Done
         return self
 
+    # TODO This init method needs proper docs, which includes every optional argument. See my comment above. 
     def __init__(self, image, ivar=None, nimg=None, amp_img=None, det_img=None, rn2img=None,
                  base_var=None, img_scale=None, fullmask=None, detector=None, spat_flexure=None,
                  filename=None, PYP_SPEC=None, units=None, exptime=None, noise_floor=None, 

pypeit/images/rawimage.py

@@ -676,9 +676,9 @@ def process(self, par, bpm=None, scattlight=None, flatimages=None, bias=None, sl
                                               exptime=self.exptime,
                                               noise_floor=self.par['noise_floor'],
                                               shot_noise=self.par['shot_noise'],
-                                              bpm=_bpm.astype(bool))
+                                              bpm=_bpm.astype(bool), 
+                                              filename=self.filename)
 
-        pypeitImage.filename = self.filename
         pypeitImage.rawheadlist = self.headarr
         pypeitImage.process_steps = [key for key in self.steps.keys() if self.steps[key]]
 

pypeit/spectrographs/jwst_nirspec.py

@@ -254,22 +254,6 @@ def allowed_mosaics(self):
             ``PypeIt``.
         """
         return [(1,2)]
-
-
-
-    def get_lamps_status(self, headarr):
-        """
-        Return a string containing the information on the lamp status.
-
-        Args:
-            headarr (:obj:`list`):
-                A list of 1 or more `astropy.io.fits.Header`_ objects.
-
-        Returns:
-            :obj:`str`: A string that uniquely represents the lamp status.
-        """
-        # TODO: JFH This is a hack to deal with the usage of this method in combineimage, which is not where it should be. 
-        return None
 
     def get_rawimage(self, raw_file, det):
         """