DM-31924: deprecate aspects of the data ID packing system

doc/changes/DM-31924.removal.md

@@ -0,0 +1,3 @@
+Deprecate methods for constructing or using `DimensionPacker` instances.
+
+The `DimensionPacker` interface is not being removed, but all concrete implementations will now be downstream of `daf_butler` and will not satisfy the assumptions of the current interfaces for constructing them.

python/lsst/daf/butler/core/dimensions/_config.py

@@ -23,6 +23,7 @@
 
 __all__ = ("DimensionConfig",)
 
+import warnings
 from collections.abc import Iterator, Mapping, Sequence
 from typing import Any
 
@@ -236,6 +237,9 @@ def _extractTopologyVisitors(self) -> Iterator[DimensionConstructionVisitor]:
                     members=members,
                 )
 
+    # TODO: remove this method and callers on DM-38687.
+    # Note that the corresponding entries in the dimensions config should
+    # not be removed at that time, because that's formally a schema migration.
     def _extractPackerVisitors(self) -> Iterator[DimensionConstructionVisitor]:
         """Process the 'packers' section of the configuration.
 
@@ -247,13 +251,16 @@ def _extractPackerVisitors(self) -> Iterator[DimensionConstructionVisitor]:
             Object that adds a `DinmensionPackerFactory` to an
             under-construction `DimensionUniverse`.
         """
-        for name, subconfig in self["packers"].items():
-            yield DimensionPackerConstructionVisitor(
-                name=name,
-                clsName=subconfig["cls"],
-                fixed=subconfig["fixed"],
-                dimensions=subconfig["dimensions"],
-            )
+        with warnings.catch_warnings():
+            # Don't warn when deprecated code calls other deprecated code.
+            warnings.simplefilter("ignore", FutureWarning)
+            for name, subconfig in self["packers"].items():
+                yield DimensionPackerConstructionVisitor(
+                    name=name,
+                    clsName=subconfig["cls"],
+                    fixed=subconfig["fixed"],
+                    dimensions=subconfig["dimensions"],
+                )
 
     def makeBuilder(self) -> DimensionConstructionBuilder:
         """Construct a `DinmensionConstructionBuilder`.

python/lsst/daf/butler/core/dimensions/_coordinate.py

@@ -45,6 +45,7 @@
     overload,
 )
 
+from deprecated.sphinx import deprecated
 from lsst.sphgeom import IntersectionRegion, Region
 from pydantic import BaseModel
 
@@ -649,6 +650,12 @@ def pack(self, name: str, *, returnMaxBits: Literal[True]) -> Tuple[int, int]:
     def pack(self, name: str, *, returnMaxBits: Literal[False]) -> int:
         ...
 
+    # TODO: Remove this method and its overloads above on DM-38687.
+    @deprecated(
+        "Deprecated in favor of configurable dimension packers.  Will be removed after v27.",
+        version="v26",
+        category=FutureWarning,
+    )
     def pack(self, name: str, *, returnMaxBits: bool = False) -> Union[Tuple[int, int], int]:
         """Pack this data ID into an integer.
 

python/lsst/daf/butler/core/dimensions/_packer.py

@@ -23,9 +23,11 @@
 
 __all__ = ("DimensionPacker",)
 
+import warnings
 from abc import ABCMeta, abstractmethod
 from typing import TYPE_CHECKING, AbstractSet, Any, Iterable, Optional, Tuple, Type, Union
 
+from deprecated.sphinx import deprecated
 from lsst.utils import doImportType
 
 from ._coordinate import DataCoordinate, DataId
@@ -47,7 +49,8 @@
     fixed : `DataCoordinate`
         Expanded data ID for the dimensions whose values must remain fixed
         (to these values) in all calls to `pack`, and are used in the results
-        of calls to `unpack`.  ``fixed.hasRecords()`` must return `True`.
+        of calls to `unpack`.  Subclasses are permitted to require that
+        ``fixed.hasRecords()`` return `True`.
     dimensions : `DimensionGraph`
         The dimensions of data IDs packed by this instance.
     """
@@ -95,7 +98,7 @@
         raise NotImplementedError()
 
     def pack(
-        self, dataId: DataId, *, returnMaxBits: bool = False, **kwargs: Any
+        self, dataId: DataId | None = None, *, returnMaxBits: bool = False, **kwargs: Any
     ) -> Union[Tuple[int, int], int]:
         """Pack the given data ID into a single integer.
 
@@ -124,7 +127,11 @@
         Should not be overridden by derived class
         (`~DimensionPacker._pack` should be overridden instead).
         """
-        dataId = DataCoordinate.standardize(dataId, **kwargs)
+        dataId = DataCoordinate.standardize(
+            dataId, **kwargs, universe=self.fixed.universe, defaults=self.fixed
+        )
+        if dataId.subset(self.fixed.graph) != self.fixed:
+            raise ValueError(f"Data ID packer expected a data ID consistent with {self.fixed}, got {dataId}.")
         packed = self._pack(dataId)
         if returnMaxBits:
             return packed, self.maxBits
@@ -159,14 +166,20 @@
     (`DataCoordinate`)
 
     The packed ID values are only unique and reversible with these
-    dimensions held fixed.  ``fixed.hasRecords() is True`` is guaranteed.
+    dimensions held fixed.
     """
 
     dimensions: DimensionGraph
     """The dimensions of data IDs packed by this instance (`DimensionGraph`).
     """
 
 
+# TODO: Remove this class on DM-38687.
+@deprecated(
+    "Deprecated in favor of configurable dimension packers.  Will be removed after v27.",
+    version="v26",
+    category=FutureWarning,
+)
 class DimensionPackerFactory:
     """A factory class for `DimensionPacker` instances.
 
@@ -228,6 +241,12 @@
         return self._cls(fixed, self._dimensions)
 
 
+# TODO: Remove this class on DM-38687.
+@deprecated(
+    "Deprecated in favor of configurable dimension packers.  Will be removed after v27.",
+    version="v26",
+    category=FutureWarning,
+)
 class DimensionPackerConstructionVisitor(DimensionConstructionVisitor):
     """Builder visitor for a single `DimensionPacker`.
 
@@ -264,8 +283,11 @@
 
     def visit(self, builder: DimensionConstructionBuilder) -> None:
         # Docstring inherited from DimensionConstructionVisitor.
-        builder.packers[self.name] = DimensionPackerFactory(
-            clsName=self._clsName,
-            fixed=self._fixed,
-            dimensions=self._dimensions,
-        )
+        with warnings.catch_warnings():
+            # Don't warn when deprecated code calls other deprecated code.
+            warnings.simplefilter("ignore", FutureWarning)
+            builder.packers[self.name] = DimensionPackerFactory(
+                clsName=self._clsName,
+                fixed=self._fixed,
+                dimensions=self._dimensions,
+            )

python/lsst/daf/butler/core/dimensions/_universe.py

@@ -42,6 +42,7 @@
     Union,
 )
 
+from deprecated.sphinx import deprecated
 from lsst.utils.classes import cached_getter, immutable
 
 from .._topology import TopologicalFamily, TopologicalSpace
@@ -459,6 +460,12 @@ def sorted(self, elements: Iterable[Union[E, str]], *, reverse: bool = False) ->
         # passed it was Dimensions; we know better.
         return result  # type: ignore
 
+    # TODO: Remove this method on DM-38687.
+    @deprecated(
+        "Deprecated in favor of configurable dimension packers.  Will be removed after v27.",
+        version="v26",
+        category=FutureWarning,
+    )
     def makePacker(self, name: str, dataId: DataCoordinate) -> DimensionPacker:
         """Make a dimension packer.
 

python/lsst/daf/butler/instrument.py

@@ -21,9 +21,17 @@
 
 __all__ = ["ObservationDimensionPacker"]
 
+# TODO: Remove this entire module on DM-38687.
+
+from deprecated.sphinx import deprecated
 from lsst.daf.butler import DataCoordinate, DimensionGraph, DimensionPacker
 
 
+@deprecated(
+    "Deprecated in favor of configurable dimension packers.  Will be removed after v27.",
+    version="v26",
+    category=FutureWarning,
+)
 class ObservationDimensionPacker(DimensionPacker):
     """A `DimensionPacker` for visit+detector or exposure+detector, given an
     instrument.

python/lsst/daf/butler/registry/tests/_registry.py

@@ -1058,19 +1058,6 @@ def testInstrumentDimensions(self):
         # with empty expression
         rows = registry.queryDataIds(dimensions, datasets=rawType, collections=run1).expanded().toSet()
         self.assertEqual(len(rows), 4 * 3)  # 4 exposures times 3 detectors
-        for dataId in rows:
-            self.assertCountEqual(dataId.keys(), ("instrument", "detector", "exposure", "visit"))
-            packer1 = registry.dimensions.makePacker("visit_detector", dataId)
-            packer2 = registry.dimensions.makePacker("exposure_detector", dataId)
-            self.assertEqual(
-                packer1.unpack(packer1.pack(dataId)),
-                DataCoordinate.standardize(dataId, graph=packer1.dimensions),
-            )
-            self.assertEqual(
-                packer2.unpack(packer2.pack(dataId)),
-                DataCoordinate.standardize(dataId, graph=packer2.dimensions),
-            )
-            self.assertNotEqual(packer1.pack(dataId), packer2.pack(dataId))
         self.assertCountEqual(set(dataId["exposure"] for dataId in rows), (100, 101, 110, 111))
         self.assertCountEqual(set(dataId["visit"] for dataId in rows), (10, 11))
         self.assertCountEqual(set(dataId["detector"] for dataId in rows), (1, 2, 3))

tests/test_dimensions.py

@@ -21,6 +21,7 @@
 
 import copy
 import itertools
+import math
 import os
 import pickle
 import unittest
@@ -37,6 +38,7 @@
     Dimension,
     DimensionConfig,
     DimensionGraph,
+    DimensionPacker,
     DimensionUniverse,
     NamedKeyDict,
     NamedValueSet,
@@ -72,6 +74,37 @@ def loadDimensionData() -> DataCoordinateSequence:
     return registry.queryDataIds(dimensions).expanded().toSequence()
 
 
+class TestDimensionPacker(DimensionPacker):
+    """A concrete `DimensionPacker` for testing its base class implementations.
+
+    This class just returns the detector ID as-is.
+    """
+
+    def __init__(self, fixed: DataCoordinate, dimensions: DimensionGraph):
+        super().__init__(fixed, dimensions)
+        self._n_detectors = fixed.records["instrument"].detector_max
+        self._max_bits = (self._n_detectors - 1).bit_length()
+
+    @property
+    def maxBits(self) -> int:
+        # Docstring inherited from DimensionPacker.maxBits
+        return self._max_bits
+
+    def _pack(self, dataId: DataCoordinate) -> int:
+        # Docstring inherited from DimensionPacker._pack
+        return dataId["detector"]
+
+    def unpack(self, packedId: int) -> DataCoordinate:
+        # Docstring inherited from DimensionPacker.unpack
+        return DataCoordinate.standardize(
+            {
+                "instrument": self.fixed["instrument"],
+                "detector": packedId,
+            },
+            graph=self.dimensions,
+        )
+
+
 class DimensionTestCase(unittest.TestCase):
     """Tests for dimensions.
 
@@ -827,6 +860,22 @@ def testSetOperations(self):
         self.assertEqual(a ^ b, a.symmetric_difference(b))
         self.assertGreaterEqual(a ^ b, (a | b) - (a & b))
 
+    def testPackers(self):
+        (instrument_data_id,) = self.allDataIds.subset(
+            self.allDataIds.universe.extract(["instrument"])
+        ).toSet()
+        (detector_data_id,) = self.randomDataIds(n=1).subset(self.allDataIds.universe.extract(["detector"]))
+        packer = TestDimensionPacker(instrument_data_id, detector_data_id.graph)
+        packed_id, max_bits = packer.pack(detector_data_id, returnMaxBits=True)
+        self.assertEqual(packed_id, detector_data_id["detector"])
+        self.assertEqual(max_bits, packer.maxBits)
+        self.assertEqual(
+            max_bits, math.ceil(math.log2(instrument_data_id.records["instrument"].detector_max))
+        )
+        self.assertEqual(packer.pack(detector_data_id), packed_id)
+        self.assertEqual(packer.pack(detector=detector_data_id["detector"]), detector_data_id["detector"])
+        self.assertEqual(packer.unpack(packed_id), detector_data_id)
+
 
 if __name__ == "__main__":
     unittest.main()