Merge pull request #13 from lsst/tickets/DM-8230

obs_base/DM-8230: Strip skymap data ID keys from registry queries.

python/lsst/obs/base/mapping.py

@@ -22,6 +22,7 @@
 
 from builtins import zip
 from builtins import object
+from collections import OrderedDict
 import os
 import re
 from lsst.daf.persistence import ButlerLocation
@@ -162,31 +163,68 @@ def lookup(self, properties, dataId):
         if self.registry is None:
             raise RuntimeError("No registry for lookup")
 
+        skyMapKeys = ("tract", "patch")
+
         where = []
         values = []
+
+        # Prepare to remove skymap entries from properties list.  These must
+        # be in the data ID, so we store which ones we're removing and create
+        # an OrderedDict that tells us where to re-insert them.  That maps the
+        # name of the property to either its index in the properties list
+        # *after* the skymap ones have been removed (for entries that aren't
+        # skymap ones) or the value from the data ID (for those that are).
+        removed = set()
+        substitutions = OrderedDict()
+        index = 0
+        properties = list(properties)  # don't modify the original list
+        for p in properties:
+            if p in skyMapKeys:
+                try:
+                    substitutions[p] = dataId[p]
+                    removed.add(p)
+                except KeyError:
+                    raise RuntimeError(
+                        "Cannot look up skymap key '%s'; it must be explicitly included in the data ID" % p
+                    )
+            else:
+                substitutions[p] = index
+                index += 1
+        # Can't actually remove while iterating above, so we do it here.
+        for p in removed:
+            properties.remove(p)
+
         fastPath = True
         for p in properties:
             if p not in ('filter', 'expTime', 'taiObs'):
                 fastPath = False
                 break
         if fastPath and 'visit' in dataId and "raw" in self.tables:
             lookupDataId = {'visit': dataId['visit']}
-            self.registry.lookup(properties, 'raw_visit', lookupDataId)
-        if dataId is not None:
-            for k, v in dataId.items():
-                if self.columns and k not in self.columns:
-                    continue
-                if k == self.obsTimeName:
-                    continue
-                where.append((k, '?'))
-                values.append(v)
-
-        lookupDataId = {k[0]: v for k, v in zip(where, values)}
-        if self.range:
-            # format of self.range is ('?', isBetween-lowKey, isBetween-highKey)
-            # here we transform that to {(lowKey, highKey): value}
-            lookupDataId[(self.range[1], self.range[2])] = dataId[self.obsTimeName]
-        return self.registry.lookup(properties, self.tables, lookupDataId, template=self.template)
+            result = self.registry.lookup(properties, 'raw_visit', lookupDataId, template=self.template)
+        else:
+            if dataId is not None:
+                for k, v in dataId.items():
+                    if self.columns and k not in self.columns:
+                        continue
+                    if k == self.obsTimeName:
+                        continue
+                    if k in skyMapKeys:
+                        continue
+                    where.append((k, '?'))
+                    values.append(v)
+            lookupDataId = {k[0]: v for k, v in zip(where, values)}
+            if self.range:
+                # format of self.range is ('?', isBetween-lowKey, isBetween-highKey)
+                # here we transform that to {(lowKey, highKey): value}
+                lookupDataId[(self.range[1], self.range[2])] = dataId[self.obsTimeName]
+            result = self.registry.lookup(properties, self.tables, lookupDataId, template=self.template)
+        if not removed:
+            return result
+        # Iterate over the query results, re-inserting the skymap entries.
+        result = [tuple(v if k in removed else item[v] for k, v in substitutions.items())
+                  for item in result]
+        return result
 
     def have(self, properties, dataId):
         """Returns whether the provided data identifier has all

tests/MinMapper2.paf

@@ -71,4 +71,11 @@ datasets: {
         storage:     "FitsCatalogStorage"
         tables:      "raw"
     }
+    forced_src: {
+        template:    "%(tract)d/%(filter)s/forced_src-v%(visit)d-c%(ccd)02d.boost"
+        python:      "lsst.afw.table.SourceCatalog"
+        persistable: "SourceCatalog"
+        storage:     "FitsCatalogStorage"
+        tables:      "raw"
+    }
 }

tests/testCameraMapper.py

@@ -169,6 +169,8 @@ def testGetDatasetTypes(self):
                               "some", "some_filename", "some_md", "some_sub",
                               "someCatalog", "someCatalog_md", "someCatalog_filename",
                               "someCatalog_len", "someCatalog_schema",
+                              "forced_src", "forced_src_md", "forced_src_filename",
+                              "forced_src_len","forced_src_schema",
                               "other_sub", "other_filename", "other_md", "other",
                               "someGz", "someGz_filename", "someFz", "someFz_filename", "someGz_md",
                               "someFz_sub", "someFz_md", "someGz_sub"
@@ -346,6 +348,20 @@ def testParentSearch(self):
         self.assertEqual(paths, [os.path.join(testDir,
                                               os.path.join('testParentSearch', '_parent', 'baz.fits[1]'))])
 
+    def testSkymapLookups(self):
+        """Test that metadata lookups don't try to get skymap data ID values from the registry.
+        """
+        butler = dafPersist.Butler(mapper=self.mapper)
+        with self.assertRaises(RuntimeError) as manager:
+            butler.dataRef("forced_src", visit=787650, ccd=13)
+            self.assertIn("Cannot lookup skymap key 'tract'", str(manager.exception))
+        # We're mostly concerned that the statements below will raise an exception;
+        # if they don't, it's not likely the following tests will fail.
+        subset = butler.subset("forced_src", visit=787650, ccd=13, tract=0)
+        self.assertEqual(len(subset), 1)
+        dataRef = butler.dataRef("forced_src", visit=787650, ccd=13, tract=0)
+        self.assertFalse(dataRef.datasetExists("forced_src"))
+
 
 class Mapper3TestCase(unittest.TestCase):
     """A test case for a mapper subclass which does not assign packageName."""