Address review comments

bin.src/makeGen3Skymap.py

@@ -24,6 +24,7 @@
 import os.path
 import sys
 
+from lsst.daf.butler import Butler
 from lsst.pipe.tasks.makeGen3SkyMap import MakeGen3SkyMapConfig, MakeGen3SkyMapTask
 
 # Build a parser for command line arguments
@@ -49,5 +50,6 @@
     config.load(args.configFile)
 
 # Construct the SkyMap Creation task and run it
-skyMapTask = MakeGen3SkyMapTask(config=config, butler=args.butler, collection=args.collection)
-skyMapTask.run()
+skyMapTask = MakeGen3SkyMapTask(config=config)
+butler = Butler(args.butler, run=args.collection)
+skyMapTask.run(butler)

python/lsst/pipe/tasks/makeGen3SkyMap.py

@@ -18,10 +18,9 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <https://www.gnu.org/licenses/>.
-import lsst.afw.geom as afwGeom
 import lsst.pex.config as pexConfig
 import lsst.pipe.base as pipeBase
-from lsst.daf.butler import Butler, DatasetType
+from lsst.daf.butler import DatasetType
 from lsst.skymap import skyMapRegistry
 
 
@@ -43,12 +42,6 @@ class MakeGen3SkyMapConfig(pexConfig.Config):
         doc="type of skyMap",
         default="dodeca",
     )
-    doWrite = pexConfig.Field(
-        doc="persist the skyMap? If False then run generates the sky map and returns it, "
-            "but does not save it to the data repository",
-        dtype=bool,
-        default=True,
-    )
 
     def validate(self):
         if self.name is None:
@@ -68,64 +61,38 @@ class MakeGen3SkyMapTask(pipeBase.Task):
     config : `MakeGen3SkyMapConfig` or None
         Instance of a configuration class specifying task options, a default
         config is created if value is None
-    butler : `str`
-        Path to a gen3 butler
-    collection : `str`
-        The name of the collection which the created skymap will be inserted
     """
 
-    def __init__(self, *, config=None, butler=None, collection=None, **kwargs):
-        if butler is None:
-            raise ValueError("A path to a butler must be given")
-        if collection is None:
-            raise ValueError("The collection to use with this butler must be provided")
+    def __init__(self, *, config=None, **kwargs):
         super().__init__(config=config, **kwargs)
-        self.butler = Butler(butler, run=collection)
-        self.collection = collection
 
-    def run(self):
-        """Construct and optionally save a SkyMap into a gen3 repository"""
+    def run(self, butler):
+        """Construct and optionally save a SkyMap into a gen3 repository
+        Parameters
+        ----------
+        butler : `lsst.daf.butler.Butler`
+            Butler repository to which the new skymap will be written
+        """
         skyMap = self.config.skyMap.apply()
-        self.logSkyMapInfo(skyMap)
+        skyMap.logSkyMapInfo(self.log)
         skyMapHash = skyMap.getSha1()
-        if self.config.doWrite:
-            try:
-                existing, = self.butler.registry.query("SELECT skymap FROM skymap WHERE hash=:hash",
-                                                       hash=skyMapHash)
-                raise RuntimeError(
-                    (f"SkyMap with name {existing.name} and hash {skyMapHash} already exist in "
-                     f"the butler collection {self.collection}, SkyMaps must be unique within "
-                     "a collection")
-                )
-            except ValueError:
-                self.log.info(f"Inserting SkyMap {self.config.name} with hash={skyMapHash}")
-                with self.butler.registry.transaction():
-                    skyMap.register(self.config.name, self.butler.registry)
-                    self.butler.registry.registerDatasetType(DatasetType(name=self.config.datasetTypeName,
-                                                                         dimensions=["skymap"],
-                                                                         storageClass="SkyMap"))
-                    self.butler.put(skyMap, self.config.datasetTypeName, {"skymap": self.config.name})
+        try:
+            existing, = butler.registry.query("SELECT skymap FROM skymap WHERE hash=:hash",
+                                              hash=skyMapHash)
+            raise RuntimeError(
+                (f"SkyMap with name {existing.name} and hash {skyMapHash} already exist in "
+                 f"the butler collection {self.collection}, SkyMaps must be unique within "
+                 "a collection")
+            )
+        except ValueError:
+            self.log.info(f"Inserting SkyMap {self.config.name} with hash={skyMapHash}")
+            with butler.registry.transaction():
+                skyMap.register(self.config.name, butler.registry)
+                butler.registry.registerDatasetType(DatasetType(name=self.config.datasetTypeName,
+                                                                dimensions=["skymap"],
+                                                                storageClass="SkyMap"))
+                butler.put(skyMap, self.config.datasetTypeName, {"skymap": self.config.name})
 
         return pipeBase.Struct(
             skyMap=skyMap
         )
-
-    def logSkyMapInfo(self, skyMap):
-        """!Log information about a sky map
-        @param[in] skyMap  sky map (an lsst.skyMap.SkyMap)
-        """
-        self.log.info("sky map has %s tracts" % (len(skyMap),))
-        for tractInfo in skyMap:
-            wcs = tractInfo.getWcs()
-            posBox = afwGeom.Box2D(tractInfo.getBBox())
-            pixelPosList = (
-                posBox.getMin(),
-                afwGeom.Point2D(posBox.getMaxX(), posBox.getMinY()),
-                posBox.getMax(),
-                afwGeom.Point2D(posBox.getMinX(), posBox.getMaxY()),
-            )
-            skyPosList = [wcs.pixelToSky(pos).getPosition(afwGeom.degrees) for pos in pixelPosList]
-            posStrList = ["(%0.3f, %0.3f)" % tuple(skyPos) for skyPos in skyPosList]
-            self.log.info("tract %s has corners %s (RA, Dec deg) and %s x %s patches" %
-                          (tractInfo.getId(), ", ".join(posStrList),
-                           tractInfo.getNumPatches()[0], tractInfo.getNumPatches()[1]))