Merge pull request #946 from lsst/tickets/DM-44802

DM-44802: Fix missing test coverage in CalibrateImage

Author: parejkoj

python/lsst/pipe/tasks/calibrateImage.py

@@ -139,16 +139,20 @@ class CalibrateImageConnections(pipeBase.PipelineTaskConnections,
 
     def __init__(self, *, config=None):
         super().__init__(config=config)
-        if not config.optional_outputs:
+        if config.optional_outputs is None or "psf_stars" not in config.optional_outputs:
             del self.psf_stars
+        if config.optional_outputs is None or "psf_stars_footprints" not in config.optional_outputs:
             del self.psf_stars_footprints
+        if config.optional_outputs is None or "astrometry_matches" not in config.optional_outputs:
             del self.astrometry_matches
+        if config.optional_outputs is None or "photometry_matches" not in config.optional_outputs:
             del self.photometry_matches
 
 
 class CalibrateImageConfig(pipeBase.PipelineTaskConfig, pipelineConnections=CalibrateImageConnections):
     optional_outputs = pexConfig.ListField(
-        doc="Which optional outputs to save (as their connection name)?",
+        doc="Which optional outputs to save (as their connection name)?"
+            " If None, do not output any of these datasets.",
         dtype=str,
         # TODO: note somewhere to disable this for benchmarking, but should
         # we always have it on for production runs?
@@ -526,7 +530,7 @@ def run(self, *, exposures, id_generator=None, result=None):
         result.stars = result.stars_footprints.asAstropy()
 
         astrometry_matches, astrometry_meta = self._fit_astrometry(result.exposure, result.stars_footprints)
-        if self.config.optional_outputs:
+        if self.config.optional_outputs is not None and "astrometry_matches" in self.config.optional_outputs:
             result.astrometry_matches = lsst.meas.astrom.denormalizeMatches(astrometry_matches,
                                                                             astrometry_meta)
 
@@ -535,20 +539,20 @@ def run(self, *, exposures, id_generator=None, result=None):
                                                                                result.stars_footprints)
         # fit_photometry returns a new catalog, so we need a new astropy table view.
         result.stars = result.stars_footprints.asAstropy()
-        if self.config.optional_outputs:
+        if self.config.optional_outputs is not None and "photometry_matches" in self.config.optional_outputs:
             result.photometry_matches = lsst.meas.astrom.denormalizeMatches(photometry_matches,
                                                                             photometry_meta)
 
         self._summarize(result.exposure, result.stars_footprints, result.background)
 
         return result
 
-    def _handle_snaps(self, exposure):
+    def _handle_snaps(self, exposures):
         """Combine two snaps into one exposure, or return a single exposure.
 
         Parameters
         ----------
-        exposure : `lsst.afw.image.Exposure` or `list` [`lsst.afw.image.Exposure]`
+        exposures : `lsst.afw.image.Exposure` or `list` [`lsst.afw.image.Exposure]`
             One or two exposures to combine as snaps.
 
         Returns
@@ -561,17 +565,20 @@ def _handle_snaps(self, exposure):
         RuntimeError
             Raised if input does not contain either 1 or 2 exposures.
         """
-        if isinstance(exposure, lsst.afw.image.Exposure):
-            return exposure
+        if isinstance(exposures, lsst.afw.image.Exposure):
+            return exposures
 
-        if isinstance(exposure, collections.abc.Sequence):
-            match len(exposure):
+        if isinstance(exposures, collections.abc.Sequence) and not isinstance(exposures, str):
+            match len(exposures):
                 case 1:
-                    return exposure[0]
+                    return exposures[0]
                 case 2:
-                    return self.snap_combine.run(exposure[0], exposure[1]).exposure
+                    return self.snap_combine.run(exposures[0], exposures[1]).exposure
                 case n:
                     raise RuntimeError(f"Can only process 1 or 2 snaps, not {n}.")
+        else:
+            raise RuntimeError("`exposures` must be either an afw Exposure (single snap visit), or a "
+                               "list/tuple of one or two of them.")
 
     def _compute_psf(self, exposure, id_generator):
         """Find bright sources detected on an exposure and fit a PSF model to

tests/test_calibrateImage.py

@@ -207,6 +207,22 @@ def test_run_2_snaps(self):
 
         self._check_run(calibrate, result)
 
+    def test_run_no_optionals(self):
+        """Test that disabling optional outputs removes them from the output
+        struct, as appropriate.
+        """
+        self.config.optional_outputs = None
+        calibrate = CalibrateImageTask(config=self.config)
+        calibrate.astrometry.setRefObjLoader(self.ref_loader)
+        calibrate.photometry.match.setRefObjLoader(self.ref_loader)
+        result = calibrate.run(exposures=self.exposure)
+
+        self._check_run(calibrate, result)
+        # These are the only optional outputs that require extra computation,
+        # the others are included in the output struct regardless.
+        self.assertNotIn("astrometry_matches", result.getDict())
+        self.assertNotIn("photometry_matches", result.getDict())
+
     def test_handle_snaps(self):
         calibrate = CalibrateImageTask(config=self.config)
         self.assertEqual(calibrate._handle_snaps(self.exposure), self.exposure)
@@ -216,6 +232,8 @@ def test_handle_snaps(self):
             calibrate._handle_snaps([])
         with self.assertRaisesRegex(RuntimeError, "Can only process 1 or 2 snaps, not 3."):
             calibrate._handle_snaps(3*[self.exposure])
+        with self.assertRaisesRegex(RuntimeError, "must be either an afw Exposure"):
+            calibrate._handle_snaps("")
 
     def test_compute_psf(self):
         """Test that our brightest sources are found by _compute_psf(),
@@ -541,30 +559,39 @@ def test_runQuantum_2_snaps(self):
         self.assertEqual(mock_run.call_args.kwargs.keys(), {"exposures", "result", "id_generator"})
 
     def test_runQuantum_no_optional_outputs(self):
-        config = CalibrateImageTask.ConfigClass()
-        config.optional_outputs = None
-        task = CalibrateImageTask(config=config)
-        lsst.pipe.base.testUtils.assertValidInitOutput(task)
-
-        quantum = lsst.pipe.base.testUtils.makeQuantum(
-            task, self.butler, self.visit_id,
-            {"exposures": [self.exposure0_id],
-             "astrometry_ref_cat": [self.htm_id],
-             "photometry_ref_cat": [self.htm_id],
-             # outputs
-             "exposure": self.visit_id,
-             "stars": self.visit_id,
-             "stars_footprints": self.visit_id,
-             "applied_photo_calib": self.visit_id,
-             "background": self.visit_id,
-             })
-        mock_run = lsst.pipe.base.testUtils.runTestQuantum(task, self.butler, quantum)
-
-        # Ensure the reference loaders have been configured.
-        self.assertEqual(task.astrometry.refObjLoader.name, "gaia_dr3_20230707")
-        self.assertEqual(task.photometry.match.refObjLoader.name, "ps1_pv3_3pi_20170110")
-        # Check that the proper kwargs are passed to run().
-        self.assertEqual(mock_run.call_args.kwargs.keys(), {"exposures", "result", "id_generator"})
+        # All the possible connections: we modify this to test each one by
+        # popping off the removed connection, then re-setting it.
+        connections = {"exposures": [self.exposure0_id, self.exposure1_id],
+                       "astrometry_ref_cat": [self.htm_id],
+                       "photometry_ref_cat": [self.htm_id],
+                       # outputs
+                       "exposure": self.visit_id,
+                       "stars": self.visit_id,
+                       "stars_footprints": self.visit_id,
+                       "background": self.visit_id,
+                       "psf_stars": self.visit_id,
+                       "psf_stars_footprints": self.visit_id,
+                       "applied_photo_calib": self.visit_id,
+                       "initial_pvi_background": self.visit_id,
+                       "astrometry_matches": self.visit_id,
+                       "photometry_matches": self.visit_id,
+                       }
+
+        # Check that we can turn off one output at a time.
+        for optional in ["psf_stars", "psf_stars_footprints", "astrometry_matches", "photometry_matches"]:
+            config = CalibrateImageTask.ConfigClass()
+            config.optional_outputs.remove(optional)
+            task = CalibrateImageTask(config=config)
+            lsst.pipe.base.testUtils.assertValidInitOutput(task)
+            # Save the removed one for the next test.
+            temp = connections.pop(optional)
+            # This will fail with "Error in connection ..." if we don't pop
+            # the optional item from the connections list just above.
+            quantum = lsst.pipe.base.testUtils.makeQuantum(task, self.butler, self.visit_id, connections)
+            # This confirms that the outputs did skip the removed one.
+            self.assertNotIn(optional, quantum.outputs)
+            # Restore the one we removed for the next test.
+            connections[optional] = temp
 
     def test_lintConnections(self):
         """Check that the connections are self-consistent.