Addressing review comments

doc/ReconstructingMeasurements.rst

@@ -83,59 +83,42 @@ id for each object.
 
     # Re-run measure on the sources selected above, using the reconstructed
     # noise replacer.
-    measTask.runPlugins(noiseReplacer, newSrcCatalog, exposure)
+    measTask.runPlugins(rebuildNoiseReplacer(exposure, srcCat), newSrcCatalog, exposure)
 
 
-At the moment, measTask.runPlugins will not run on any child
-objects if their parents are not also in the catalog. This issue can be
-solved in several ways, two of which are options in makeRerunCatalog. The
-``resetParents`` flag (True by default) will reset the parent keys of any
-children whose parents are not included in the idList, effectively turning
-them into parents. The ``addParents`` flag (False by default) will add these
-parents to the idList. Some timed examples follow:
+At the time of writing, measTask.runPlugins will not run on any child
+objects if their parents are not also in the catalog. The makeRerunCatalog
+function has two options to resolve this issue. The ``resetParents`` flag (True
+ by default) will reset the parent keys of any children whose parents are not
+  included in the idList, effectively turning them into parents. The
+``addParents`` flag (False by default) will add these parents to the idList.
 
 .. code-block:: python
 
-    # Continuing from above again
-    from timeit import Timer
-    from collections import namedtuple
-
-    timingResult = namedtuple("TimingResult",
-        ['niter', 'total', 'min', 'median', 'mean'])
-
-    # Define a convenient function for timing
-    # This excludes the time spent rebuilding the NoiseReplacer
-    def timeRunPlugins(repeat=2):
-        timer = timeit.Timer(
-            "measTask.runPlugins(noiseReplacer, newSrcCatalog, exposure)",
-            setup="from lsst.meas.base.measurementInvestigationLib "
-                  "import rebuildNoiseReplacer;"
-                  "noiseReplacer = rebuildNoiseReplacer(exposure, srcCat)",
-            globals={'exposure':exposure, 'srcCat':srcCat, 'measTask': measTask,
-                    'newSrcCatalog':newSrcCatalog}
-        )
-        times = timer.repeat(repeat,1)
-        result = timingResult(repeat, np.sum(times), np.min(times),
-                              np.median(times), np.mean(times))
-        return result
-
+    import numpy as np
     # Get only the child ids
     idsToRerun = srcCat["id"][srcCat[srcCat.getParentKey()] != 0]
 
-    # Previously, it would skip all children and finish instantaneously
+    # Previously, it would skip all children - only the noiseReplacer takes time
     newSrcCatalog = makeRerunCatalog(
         schema, srcCat, idsToRerun, fields=fields, resetParents=False
     )
-    print("Old (resetParents=False): ", timeRunPlugins())
+    measTask.runPlugins(rebuildNoiseReplacer(exposure, srcCat), newSrcCatalog, exposure)
+    # None of these objects have centroids
+    print(len(newSrcCatalog), np.sum(np.isnan(newSrcCatalog["base_NaiveCentroid_x"])))
 
-    # The new default resets parents and takes a couple of seconds
-    newSrcCatalog = makeRerunCatalog(schema, srcCat, idsToRerun, fields=fields)
-    print("New (default resetParents=True): ", timeRunPlugins())
+    # resetParents=True (default) resets parents and takes a few seconds longer
+    newSrcCatalog = makeRerunCatalog(
+        schema, srcCat, idsToRerun, fields=fields, resetParents=True
+    )
+    measTask.runPlugins(rebuildNoiseReplacer(exposure, srcCat), newSrcCatalog, exposure)
+    # Now none of the objects have nan centroids
+    print(len(newSrcCatalog), np.sum(np.isnan(newSrcCatalog["base_NaiveCentroid_x"])))
 
-    # Running the parents as well takes a little longer
+    # Running the parents as well with addParents=True takes a little longer
     newSrcCatalog = makeRerunCatalog(
-        schema, srcCat, idsToRerun, fields=fields, addParents=True,
-        resetParents=False
+        schema, srcCat, idsToRerun, fields=fields, addParents=True
     )
-    print("New (addParents=True, resetParents=False): ", timeRunPlugins())
+    measTask.runPlugins(rebuildNoiseReplacer(exposure, srcCat), newSrcCatalog, exposure)
+    print(len(newSrcCatalog), np.sum(np.isnan(newSrcCatalog["base_NaiveCentroid_x"])))
 

python/lsst/meas/base/measurementInvestigationLib.py

@@ -81,6 +81,10 @@ def makeRerunCatalog(schema, oldCatalog, idList, fields=None,
     Specifying ids in a new measurement catalog which correspond to ids in an
     old catalog makes comparing results much easier.
 
+    The resetParents and addParents options are needed because
+    lsst.meas.base.SingleFrameMeasurementTask.runPlugins() will skip child
+    objects whose parents are not in the catalog.
+
     Parameters
     ----------
     schema : lsst.afw.table.Schema
@@ -99,9 +103,8 @@ def makeRerunCatalog(schema, oldCatalog, idList, fields=None,
         will be copied from the old catalog into the new catalog.
 
     resetParents: boolean
-        Flag to indicate that child objects should have their parents set to 0.
-        Otherwise, lsst.meas.base.SingleFrameMeasurementTask.runPlugins() will
-        skip these ids unless their parents are also included in idList.
+        Flag to toggle whether child objects whose parents are not in the
+        idList should have their parents reset to zero.
 
     addParents: boolean
         Flag to toggle whether parents of child objects will be added to the
@@ -131,19 +134,15 @@ def makeRerunCatalog(schema, oldCatalog, idList, fields=None,
         if entry not in schema:
             schema.addField(oldCatalog.schema.find(entry).field)
 
-    # It's likely better to convert to a list and append
-    idList = list(idList)
-
     if addParents:
-        lenIdList = len(idList)
-        for idx in range(lenIdList):
-            srcId = idList[idx]
-            oldSrc = oldCatalog.find(srcId)
-            parent = oldSrc.getParent()
-            if parent != 0 and parent not in idList:
-                idList.append(parent)
-
-    idList.sort()
+        newIdList = set()
+        for srcId in idList:
+            newIdList.add(srcId)
+            parentId = oldCatalog.find(srcId).getParent()
+            if parentId:
+                newIdList.add(parentId)
+        idList = newIdList
+    idList = sorted(idList)
 
     measCat = SourceCatalog(schema)
     for srcId in idList:
@@ -152,7 +151,7 @@ def makeRerunCatalog(schema, oldCatalog, idList, fields=None,
         src.setId(srcId)
         src.setFootprint(oldSrc.getFootprint())
         parent = oldSrc.getParent()
-        if parent != 0 and resetParents and parent not in idList:
+        if resetParents and parent and parent not in idList:
             parent = 0
         src.setParent(parent)
         src.setCoord(oldSrc.getCoord())