feat(data): carry the concrete type inside serialized external distortion parameters

ncore/data/__init__.py

@@ -53,6 +53,7 @@
     ReferencePolynomial,
     RowOffsetStructuredSpinningLidarModelParameters,
     ShutterType,
+    register_external_distortion_parameters,
 )
 
 
@@ -76,6 +77,7 @@
     "ConcreteCameraModelParametersUnion",
     "ConcreteExternalDistortionParametersUnion",
     "ExternalDistortionParameters",
+    "register_external_distortion_parameters",
     "ConcreteLidarModelParametersUnion",
     "PointCloud",
     "LabelCategory",

ncore/impl/data/types.py

@@ -21,10 +21,11 @@
 import sys
 
 from abc import ABC, abstractmethod
-from dataclasses import dataclass, replace
+from dataclasses import dataclass, field, replace
 from enum import IntEnum, auto, unique
 from typing import (
     TYPE_CHECKING,
+    Any,
     Callable,
     ClassVar,
     Dict,
@@ -34,6 +35,8 @@
     Optional,
     Protocol,
     Tuple,
+    Type,
+    TypeVar,
     Union,
 )
 
@@ -127,7 +130,71 @@ def __post_init__(self) -> None:
             )
 
 
+#: Type-var for the external distortion parameters registrar
+ExternalDistortionParametersT = TypeVar("ExternalDistortionParametersT", bound=ExternalDistortionParameters)
+
+#: Key under which the concrete type identifier is stored *inside* the serialized external
+#: distortion parameters. The identifier has to travel with the nested object: `dataclasses_json`
+#: reconstructs whatever type the field is annotated with, so without it an abstract annotation
+#: cannot be resolved back to a concrete class.
+EXTERNAL_DISTORTION_TYPE_KEY = "external_distortion_type"
+
+#: Identifier assumed for external distortion parameters serialized before the type key existed.
+#: The bivariate windshield model was the only concrete type at that point, so untagged data is
+#: unambiguous.
+_LEGACY_EXTERNAL_DISTORTION_TYPE = "bivariate-windshield"
+
+#: Maps the serialized identifier to the concrete external distortion parameters class
+_EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE: Dict[str, Type[ExternalDistortionParameters]] = {}
+
+
+def register_external_distortion_parameters(
+    parameters_class: Type[ExternalDistortionParametersT],
+) -> Type[ExternalDistortionParametersT]:
+    """Registers a concrete external distortion parameters class for deserialization
+
+    Usable as a class decorator. The class is keyed by its :meth:`type` identifier, which is what
+    the serialized form carries, so that out-of-tree external distortion parameters round-trip
+    through :meth:`CameraModelParameters.from_dict` as well as the in-tree ones.
+    """
+    identifier = parameters_class.type()
+    existing = _EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE.get(identifier)
+    if existing is not None and existing is not parameters_class:
+        raise ValueError(f"External distortion type {identifier!r} is already registered to {existing.__name__}")
+    _EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE[identifier] = parameters_class
+    return parameters_class
+
+
+def external_distortion_parameters_field(default: Optional[ExternalDistortionParameters] = None) -> Any:
+    """Field carrying external distortion parameters as a type-tagged (discriminated) object
+
+    Encodes the concrete :meth:`ExternalDistortionParameters.type` alongside the object's own
+    fields and dispatches on it when decoding, so the field can be declared against the abstract
+    base rather than a closed union of concrete types.
+    """
+
+    def encoder(parameters: Optional[ExternalDistortionParameters]) -> Optional[Dict]:
+        if parameters is None:
+            return None
+        return {**parameters.to_dict(), EXTERNAL_DISTORTION_TYPE_KEY: parameters.type()}
+
+    def decoder(encoded: Any) -> Optional[ExternalDistortionParameters]:
+        if encoded is None or isinstance(encoded, ExternalDistortionParameters):
+            # Already-typed values are passed through: `from_dict` is not the only construction
+            # path, and callers assign concrete parameters directly.
+            return encoded
+        encoded = dict(encoded)
+        identifier = encoded.pop(EXTERNAL_DISTORTION_TYPE_KEY, _LEGACY_EXTERNAL_DISTORTION_TYPE)
+        parameters_class = _EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE.get(identifier)
+        if parameters_class is None:
+            raise ValueError(f"Unknown external distortion type: {identifier}")
+        return parameters_class.from_dict(encoded)
+
+    return field(default=default, metadata=dataclasses_json.config(encoder=encoder, decoder=decoder))
+
+
 @dataclass
+@register_external_distortion_parameters
 class BivariateWindshieldModelParameters(ExternalDistortionParameters):
     """Represents parameters required to create a windshield external distortion model"""
 
@@ -190,9 +257,9 @@ class CameraModelParameters(dataclasses_json.DataClassJsonMixin, ABC):
     )  #: Width and height of the image in pixels (uint64, [2,])
     shutter_type: ShutterType = util.enum_field(ShutterType)  #: Shutter type of the camera's imaging sensor
 
-    external_distortion_parameters: Optional[ConcreteExternalDistortionParametersUnion] = (
-        None  #: Optional external distortion source associated to the camera (e.g. windshield). If a source exists, rays will be distorted prior to reaching the camera and its associated lens distortion if applicable
-    )
+    external_distortion_parameters: Optional[ExternalDistortionParameters] = (
+        external_distortion_parameters_field()
+    )  #: Optional external distortion source associated to the camera (e.g. windshield). If a source exists, rays will be distorted prior to reaching the camera and its associated lens distortion if applicable
 
     @abstractmethod
     def transform(
@@ -236,7 +303,7 @@ def __post_init__(self) -> None:
             self.shutter_type = ShutterType(self.shutter_type)
         assert self.shutter_type in ShutterType.__members__.values()
 
-        assert isinstance(self.external_distortion_parameters, (type(None), ConcreteExternalDistortionParametersUnion))
+        assert isinstance(self.external_distortion_parameters, (type(None), ExternalDistortionParameters))
 
 
 @dataclass
@@ -860,9 +927,11 @@ def encode_camera_model_parameters(camera_model_parameters: ConcreteCameraModelP
         "camera_model_parameters": camera_model_parameters.to_dict(),
     }
 
-    # Store type of external distortion, if available
+    # The type also travels inside the serialized parameters (see EXTERNAL_DISTORTION_TYPE_KEY).
+    # It is kept here as well so that readers predating the nested tag keep resolving the concrete
+    # type; drop this once no such reader remains.
     if camera_model_parameters.external_distortion_parameters:
-        encoded["external_distortion_type"] = camera_model_parameters.external_distortion_parameters.type()
+        encoded[EXTERNAL_DISTORTION_TYPE_KEY] = camera_model_parameters.external_distortion_parameters.type()
 
     return encoded
 
@@ -875,15 +944,18 @@ def decode_camera_model_parameters(encoded_parameters: Mapping) -> ConcreteCamer
     # Copy as we might modify the dictionary in place
     camera_model_parameters = encoded_parameters["camera_model_parameters"].copy()
 
-    # Hook up typed external distortion type, if present
-    external_distortion_type: Optional[str] = encoded_parameters.get("external_distortion_type")
-    if external_distortion_type is not None:
-        if external_distortion_type == "bivariate-windshield":
-            camera_model_parameters["external_distortion_parameters"] = BivariateWindshieldModelParameters.from_dict(
-                camera_model_parameters["external_distortion_parameters"]
-            )
-        else:
+    # Older payloads carry the external distortion type beside the camera parameters rather than
+    # inside them; move it into the nested object so the field's own decoder can dispatch on it.
+    # Payloads written since then already carry it inside and need nothing here.
+    external_distortion_type: Optional[str] = encoded_parameters.get(EXTERNAL_DISTORTION_TYPE_KEY)
+    nested_distortion = camera_model_parameters.get("external_distortion_parameters")
+    if external_distortion_type is not None and isinstance(nested_distortion, Mapping):
+        if external_distortion_type not in _EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE:
             raise ValueError(f"Unknown external distortion type: {external_distortion_type}")
+        camera_model_parameters["external_distortion_parameters"] = {
+            **nested_distortion,
+            EXTERNAL_DISTORTION_TYPE_KEY: external_distortion_type,
+        }
 
     # Return typed camera model parameters
     if camera_model_type == "ftheta":

ncore/impl/sensors/camera.py

@@ -1319,7 +1319,7 @@ def get_parameters(self) -> types.FThetaCameraModelParameters:
             resolution=self.resolution.cpu().numpy().astype(np.uint64),
             shutter_type=self.shutter_type,
             external_distortion_parameters=cast(
-                Optional[types.ConcreteExternalDistortionParametersUnion],
+                Optional[types.ExternalDistortionParameters],
                 map_optional(self.external_distortion, lambda x: x.get_parameters()),
             ),
             principal_point=self.principal_point.cpu().numpy().astype(np.float32) - 0.5,
@@ -1547,7 +1547,7 @@ def get_parameters(self) -> types.IdealPinholeCameraModelParameters:
             resolution=self.resolution.cpu().numpy().astype(np.uint64),
             shutter_type=self.shutter_type,
             external_distortion_parameters=cast(
-                Optional[types.ConcreteExternalDistortionParametersUnion],
+                Optional[types.ExternalDistortionParameters],
                 map_optional(self.external_distortion, lambda x: x.get_parameters()),
             ),
             principal_point=self.principal_point.cpu().numpy().astype(np.float32),
@@ -1616,7 +1616,7 @@ def get_parameters(self) -> types.OpenCVPinholeCameraModelParameters:
             resolution=self.resolution.cpu().numpy().astype(np.uint64),
             shutter_type=self.shutter_type,
             external_distortion_parameters=cast(
-                Optional[types.ConcreteExternalDistortionParametersUnion],
+                Optional[types.ExternalDistortionParameters],
                 map_optional(self.external_distortion, lambda x: x.get_parameters()),
             ),
             principal_point=self.principal_point.cpu().numpy().astype(np.float32),
@@ -1852,7 +1852,7 @@ def get_parameters(self) -> types.OpenCVFisheyeCameraModelParameters:
             resolution=self.resolution.cpu().numpy().astype(np.uint64),
             shutter_type=self.shutter_type,
             external_distortion_parameters=cast(
-                Optional[types.ConcreteExternalDistortionParametersUnion],
+                Optional[types.ExternalDistortionParameters],
                 map_optional(self.external_distortion, lambda x: x.get_parameters()),
             ),
             principal_point=self.principal_point.cpu().numpy().astype(np.float32),

ncore/impl/sensors/camera_test.py

@@ -18,7 +18,7 @@
 import os
 import unittest
 
-from typing import List, Tuple, Union, cast
+from typing import Dict, List, Tuple, Union, cast
 
 import cv2
 import numpy as np
@@ -31,6 +31,7 @@
 
 from ncore.impl.common.util import unpack_optional
 from ncore.impl.data.types import (
+    EXTERNAL_DISTORTION_TYPE_KEY,
     BivariateWindshieldModelParameters,
     CameraModelParameters,
     ConcreteCameraModelParametersUnion,
@@ -2427,6 +2428,16 @@ def _windshield() -> BivariateWindshieldModelParameters:
             vertical_poly_inverse=np.array([0.0, 0.0, 1.0, 0.0, 0.0, 0.0], dtype=np.float32),
         )
 
+    @classmethod
+    def _camera_with_distortion(cls) -> IdealPinholeCameraModelParameters:
+        return IdealPinholeCameraModelParameters(
+            resolution=np.array([640, 480], dtype=np.uint64),
+            shutter_type=ShutterType.GLOBAL,
+            principal_point=np.array([320.0, 240.0], dtype=np.float32),
+            focal_length=np.array([500.0, 500.0], dtype=np.float32),
+            external_distortion_parameters=cls._windshield(),
+        )
+
     def test_dispatches_to_concrete_model(self):
         parameters = self._windshield()
         model = external_distortion_model_from_parameters(parameters, device="cpu")
@@ -2486,11 +2497,11 @@ def encode(parameters: ExternalDistortionParameters) -> dict:
             ExternalDistortionParameters.type()
 
     def test_from_dict_reconstructs_the_concrete_distortion_type(self):
-        # `dataclasses_json` constructs whatever type the field is annotated with, so this is the
-        # path that breaks if `external_distortion_parameters` names an abstract type: the nested
-        # dict would deserialize into that base instead of the concrete model's parameters, and the
-        # failure only shows up later where the value is used. Cover the raw `from_dict` path in
-        # addition to the encode/decode helpers, which construct the concrete type by hand.
+        # `dataclasses_json` constructs whatever type the field is annotated with, so with the
+        # field declared against the abstract base this only works because the serialized form
+        # carries the concrete type (EXTERNAL_DISTORTION_TYPE_KEY) and the field's decoder
+        # dispatches on it. Without that, the nested dict deserializes into the base itself and the
+        # failure only surfaces later, where the value is used.
         camera = IdealPinholeCameraModelParameters(
             resolution=np.array([640, 480], dtype=np.uint64),
             shutter_type=ShutterType.GLOBAL,
@@ -2502,19 +2513,84 @@ def test_from_dict_reconstructs_the_concrete_distortion_type(self):
         self.assertIsInstance(restored.external_distortion_parameters, BivariateWindshieldModelParameters)
         self.assertEqual(restored.to_json(), camera.to_json())
 
+    def test_serialized_form_carries_the_concrete_type(self):
+        # The discriminator has to sit inside the nested object: that is what a field declared
+        # against the abstract base has to dispatch on when reconstructing.
+        camera = self._camera_with_distortion()
+        nested: Dict = dict(cast(Dict, camera.to_dict()["external_distortion_parameters"]))
+        self.assertEqual(nested[EXTERNAL_DISTORTION_TYPE_KEY], "bivariate-windshield")
+
+    def test_untagged_legacy_payload_still_decodes(self):
+        # Payloads written before the nested type key exist in stored data; they are unambiguous
+        # because the bivariate windshield was the only concrete type at the time.
+        camera = self._camera_with_distortion()
+        encoded: Dict = dict(camera.to_dict())
+        legacy: Dict = dict(cast(Dict, encoded["external_distortion_parameters"]))
+        legacy.pop(EXTERNAL_DISTORTION_TYPE_KEY)
+        encoded["external_distortion_parameters"] = legacy
+        restored = IdealPinholeCameraModelParameters.from_dict(encoded)
+        self.assertIsInstance(restored.external_distortion_parameters, BivariateWindshieldModelParameters)
+        self.assertEqual(restored.to_json(), camera.to_json())
+
+    def test_legacy_camera_level_type_is_migrated_by_the_decoder(self):
+        # The old encoded layout stored the type beside the camera parameters, with an untagged
+        # nested object. decode_camera_model_parameters has to move it inside.
+        camera = self._camera_with_distortion()
+        encoded = encode_camera_model_parameters(camera)
+        camera_parameters: Dict = dict(cast(Dict, encoded["camera_model_parameters"]))
+        nested: Dict = dict(cast(Dict, camera_parameters["external_distortion_parameters"]))
+        nested.pop(EXTERNAL_DISTORTION_TYPE_KEY)
+        camera_parameters["external_distortion_parameters"] = nested
+        legacy: Dict = dict(encoded)
+        legacy["camera_model_parameters"] = camera_parameters
+        decoded = decode_camera_model_parameters(legacy)
+        self.assertIsInstance(decoded.external_distortion_parameters, BivariateWindshieldModelParameters)
+
+    def test_decode_accepts_old_and_new_payload_structures(self):
+        # The two serialized layouts must decode to the same parameters:
+        #
+        #   old: the type sits beside the camera parameters, the nested object is untagged
+        #   new: the type sits inside the nested object (both are written, for older readers)
+        #
+        # Stored data contains the old layout, so this equivalence is what lets the field be
+        # declared against the abstract base without a migration.
+        camera = self._camera_with_distortion()
+        new_payload = encode_camera_model_parameters(camera)
+
+        camera_parameters: Dict = dict(cast(Dict, new_payload["camera_model_parameters"]))
+        nested: Dict = dict(cast(Dict, camera_parameters["external_distortion_parameters"]))
+        self.assertIn(EXTERNAL_DISTORTION_TYPE_KEY, nested)
+        nested.pop(EXTERNAL_DISTORTION_TYPE_KEY)
+        camera_parameters["external_distortion_parameters"] = nested
+        old_payload: Dict = dict(new_payload)
+        old_payload["camera_model_parameters"] = camera_parameters
+
+        decoded_old = decode_camera_model_parameters(old_payload)
+        decoded_new = decode_camera_model_parameters(new_payload)
+
+        for decoded in (decoded_old, decoded_new):
+            self.assertIsInstance(decoded.external_distortion_parameters, BivariateWindshieldModelParameters)
+        self.assertEqual(decoded_old.to_json(), decoded_new.to_json())
+        self.assertEqual(decoded_old.to_json(), camera.to_json())
+
+    def test_unknown_serialized_type_raises(self):
+        camera = self._camera_with_distortion()
+        encoded: Dict = dict(camera.to_dict())
+        nested: Dict = dict(cast(Dict, encoded["external_distortion_parameters"]))
+        nested[EXTERNAL_DISTORTION_TYPE_KEY] = "not-a-real-distortion"
+        encoded["external_distortion_parameters"] = nested
+        with self.assertRaises(ValueError):
+            IdealPinholeCameraModelParameters.from_dict(encoded)
+
     def test_abstract_base_is_not_instantiable(self):
         # `ABC` does not prevent instantiation without an abstract method, and `type()` is
         # deliberately non-abstract, so the base guards itself explicitly.
         with self.assertRaises(TypeError):
             ExternalDistortionParameters()
 
     def test_encode_decode_roundtrip_with_distortion(self):
-        # The `external_distortion_parameters` field deliberately keeps naming the concrete union
-        # rather than the new abstract base: widening a field a caller *reads* is a breaking change
-        # for downstream consumers that pass it on to concretely-typed APIs, and it also changes
-        # what `dataclasses_json` reconstructs (see test_from_dict_reconstructs_the_concrete_
-        # distortion_type). It is left to the separate widening pass that covers the other read
-        # positions. This pins the round-trip that has to keep working either way.
+        # The round-trip through the encode/decode helpers, which carry the type beside the camera
+        # parameters as well as inside them.
         camera = IdealPinholeCameraModelParameters(
             resolution=np.array([640, 480], dtype=np.uint64),
             shutter_type=ShutterType.GLOBAL,