fix: check for transforms before checking for matching coord sys name, also docstrings (#1559)

Author: dbirman

src/aind_data_schema/utils/validators.py

@@ -106,7 +106,18 @@ def _time_validation_recurse_helper(data, acquisition_start_time, acquisition_en
 
 
 def _recurse_helper(data, **kwargs):
-    """Helper function for recursive_axis_order_check: recurse calls for lists and objects only"""
+    """Helper function for recursive coordinate system validation.
+
+    Recursively processes lists and objects, calling recursive_coord_system_check
+    on each element or attribute.
+
+    Parameters
+    ----------
+    data : Any
+        The data structure to process recursively
+    **kwargs
+        Keyword arguments passed to recursive_coord_system_check
+    """
     if isinstance(data, list):
         for item in data:
             recursive_coord_system_check(item, **kwargs)
@@ -122,42 +133,92 @@ def _recurse_helper(data, **kwargs):
 
 
 def _system_check_helper(data, coordinate_system_name: Optional[str], axis_count: Optional[int]):
-    """Helper function to raise errors if the coordinate_system_name or axis_count don't match"""
-    object_type = getattr(data, "object_type", type(data).__name__)
+    """Helper function to validate coordinate system requirements for objects with transforms.
 
-    if not coordinate_system_name or not axis_count:
-        raise ValueError(
-            f"CoordinateSystem is required when a Transform or Coordinate is present (object_type: {object_type})"
-        )
+    Only validates coordinate system requirements if the object contains transform components
+    (Translation, Rotation, or Scale). Objects without transforms don't require coordinate systems.
 
-    if data.coordinate_system_name not in coordinate_system_name:
-        raise ValueError(
-            f"System name mismatch for {object_type}, expected {coordinate_system_name}, "
-            f"found {data.coordinate_system_name}"
-        )
+    Parameters
+    ----------
+    data : object
+        The object to validate
+    coordinate_system_name : Optional[str]
+        Expected coordinate system name (can be None if no transforms present)
+    axis_count : Optional[int]
+        Expected number of axes in the coordinate system (can be None if no transforms present)
+
+    Raises
+    ------
+    ValueError
+        If coordinate system is required but missing, system name doesn't match,
+        or transform field length doesn't match axis count
+    """
+    # First check if this object has any transform components
+    has_transforms = False
+    transform_components = []
+    object_type = getattr(data, "object_type", type(data).__name__)
 
-    # Check lengths of subfields based on class types
     if hasattr(data, "__dict__"):
         for attr_name, attr_value in data.__dict__.items():
             # Check if the attribute's class name is one of the AXIS_TYPES
             if hasattr(attr_value, "__class__") and attr_value.__class__.__name__ in AXIS_TYPES:
-                # Construct the field name by converting the class name to lowercase
-                field_name = attr_value.__class__.__name__.lower()
-                sub_data = getattr(data, field_name, None)
-                # Check if the object has the corresponding field and if it's a list with correct length
-                if sub_data and hasattr(sub_data, field_name):
-                    field_value = getattr(sub_data, field_name)
-                    if len(field_value) != axis_count:
-                        raise ValueError(
-                            f"Axis count mismatch for {object_type}, expected {axis_count} axes, "
-                            f"but found {len(field_value)}"
-                        )
+                has_transforms = True
+                transform_components.append(attr_value)
+
+    # Only require coordinate system if there are transforms present
+    if has_transforms:
+        if not coordinate_system_name or not axis_count:
+            raise ValueError(
+                f"CoordinateSystem is required when a Transform or Coordinate is present (object_type: {object_type})"
+            )
+
+        if data.coordinate_system_name not in coordinate_system_name:
+            raise ValueError(
+                f"System name mismatch for {object_type}, expected {coordinate_system_name}, "
+                f"found {data.coordinate_system_name}"
+            )
+
+        # Check lengths of transform fields match axis count
+        for transform_component in transform_components:
+            field_name = transform_component.__class__.__name__.lower()
+            sub_data = getattr(data, field_name, None)
+            # Check if the object has the corresponding field and if it's a list with correct length
+            if sub_data and hasattr(sub_data, field_name):
+                field_value = getattr(sub_data, field_name)
+                if len(field_value) != axis_count:
+                    raise ValueError(
+                        f"Axis count mismatch for {object_type}, expected {axis_count} axes, "
+                        f"but found {len(field_value)}"
+                    )
 
 
 def recursive_coord_system_check(data, coordinate_system_name: Optional[str], axis_count: Optional[int]):
-    """Recursively check fields, see if they are Coordinates and check if they match a List[values]
+    """Recursively validate coordinate system requirements for objects with transforms.
 
-    Note that we just need to check if the axes all show up, not necessarily in matching order
+    Traverses the data structure and validates that objects with transform components
+    (Translation, Rotation, Scale) have the correct coordinate system name and that
+    transform field lengths match the expected axis count.
+
+    Parameters
+    ----------
+    data : Any
+        The data structure to validate recursively
+    coordinate_system_name : Optional[str]
+        Expected coordinate system name (can be None if no transforms present)
+    axis_count : Optional[int]
+        Expected number of axes in the coordinate system (can be None if no transforms present)
+
+    Raises
+    ------
+    ValueError
+        If coordinate system is required but missing, system name doesn't match,
+        or transform field length doesn't match axis count
+
+    Notes
+    -----
+    Objects without transform components are not required to have coordinate systems.
+    When a new coordinate_system is encountered in the data, it overrides the provided
+    coordinate_system_name and axis_count for subsequent validation.
     """
 
     if not data:

tests/test_utils_validators.py

@@ -215,20 +215,73 @@ def test_recursive_coord_system_check_with_axis_count_mismatch(self):
         self.assertIn("Axis count mismatch", str(context.exception))
 
     def test_recursive_coord_system_check_with_missing_coordinate_system(self):
-        """Test recursive_coord_system_check with missing coordinate system"""
+        """Test recursive_coord_system_check with missing coordinate system for object WITH transforms"""
 
-        class MockData(BaseModel):
-            """Test class"""
-
-            coordinate_system_name: str
-
-        data = MockData(coordinate_system_name=self.coordinate_system_name)
+        # Object with transforms should still require coordinate system
+        data = TranslationWrapper(
+            coordinate_system_name=self.coordinate_system_name, translation=Translation(translation=[0.5, 1])
+        )
 
         with self.assertRaises(ValueError) as context:
             recursive_coord_system_check(data, None, axis_count=0)
 
         self.assertIn("CoordinateSystem is required", str(context.exception))
 
+    def test_recursive_coord_system_check_object_without_transforms(self):
+        """Test recursive_coord_system_check with object without transforms (should not require coordinate system)"""
+
+        class ObjectWithoutTransforms(DataModel):
+            """Object without any transform components"""
+
+            coordinate_system_name: str
+            some_field: str
+
+        data = ObjectWithoutTransforms(coordinate_system_name=self.coordinate_system_name, some_field="test_value")
+
+        # Should not raise any exception even with None coordinate_system_name and axis_count
+        recursive_coord_system_check(data, None, axis_count=0)
+
+    def test_system_check_helper_object_without_transforms(self):
+        """Test _system_check_helper with object without transforms (should not require coordinate system)"""
+
+        class ObjectWithoutTransforms(DataModel):
+            """Object without any transform components"""
+
+            coordinate_system_name: str
+            some_field: str
+
+        data = ObjectWithoutTransforms(coordinate_system_name=self.coordinate_system_name, some_field="test_value")
+
+        # Should not raise any exception even with None coordinate_system_name and axis_count
+        _system_check_helper(data, None, axis_count=0)
+
+    def test_mixed_objects_with_and_without_transforms(self):
+        """Test with a mix of objects with and without transforms"""
+
+        class ObjectWithoutTransforms(DataModel):
+            """Object without any transform components"""
+
+            coordinate_system_name: str
+            some_field: str
+
+        class ContainerModel(DataModel):
+            """Container with mixed objects"""
+
+            with_transform: TranslationWrapper
+            without_transform: ObjectWithoutTransforms
+
+        container = ContainerModel(
+            with_transform=TranslationWrapper(
+                coordinate_system_name=self.coordinate_system_name, translation=Translation(translation=[0.5, 1])
+            ),
+            without_transform=ObjectWithoutTransforms(
+                coordinate_system_name="any_name", some_field="test"  # This can be anything since no transforms
+            ),
+        )
+
+        # Should pass validation - only the object with transforms is checked
+        recursive_coord_system_check(container, self.coordinate_system_name, axis_count=2)
+
 
 class MockEnum(Enum):
     """Mock Enum for testing"""