Merge pull request #254 from scipp/depect-depends_on-loops

Detect cycles in depends_on

src/scippnexus/field.py

@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright (c) 2023 Scipp contributors (https://github.com/scipp)
 # @author Simon Heybrock
+from __future__ import annotations
+
 import datetime
 import posixpath
 import re
@@ -113,7 +115,7 @@ class Field:
     """
 
     dataset: H5Dataset
-    parent: 'Group'
+    parent: Group
     sizes: dict[str, int] | None = None
     dtype: sc.DType | None = None
     errors: H5Dataset | None = None
@@ -139,7 +141,7 @@ def shape(self) -> tuple[int, ...]:
         return tuple(self.sizes.values())
 
     @cached_property
-    def file(self) -> 'Group':
+    def file(self) -> Group:
         return self.parent.file
 
     def _load_variances(self, var, index):

src/scippnexus/nxtransformations.py

@@ -308,13 +308,25 @@ def parse_depends_on_chain(
     # Use raw h5py objects to follow the chain because that avoids constructing
     # expensive intermediate snx.Group objects.
     file = parent.underlying.file
+    visited = [depends_on.absolute_path()]
     try:
         while depends_on.value != '.':
             transform, base = _locate_depends_on_target(
                 file, depends_on, parent.definitions
             )
             depends_on = DependsOn(parent=base, value=transform.attrs['depends_on'])
             chain.transformations[transform.name] = transform[()]
+            if depends_on.absolute_path() == visited[-1]:
+                depends_on.value = '.'
+                # Transform.from_object does not see the full chain, so it cannot
+                # detect this case.
+                chain.transformations[transform.name].depends_on = depends_on
+            elif depends_on.absolute_path() in visited:
+                raise ValueError(
+                    'Circular depends_on chain detected: '
+                    f'{[*visited, depends_on.absolute_path()]}'
+                )
+            visited.append(depends_on.absolute_path())
     except KeyError as e:
         warnings.warn(
             UserWarning(f'depends_on chain {depends_on} references missing node {e}'),

tests/nxtransformations_test.py

@@ -829,3 +829,136 @@ def test_compute_transformation_warns_if_transformation_missing_vector_attr(
         UserWarning, match="Invalid transformation, missing attribute 'vector'"
     ):
         root[()]
+
+
+def test_compute_positions_terminates_with_depends_on_to_self_absolute(h5root):
+    instrument = snx.create_class(h5root, 'instrument', snx.NXinstrument)
+    detector = create_detector(instrument)
+    snx.create_field(detector, 'x_pixel_offset', sc.linspace('xx', -1, 1, 2, unit='m'))
+    snx.create_field(detector, 'y_pixel_offset', sc.linspace('yy', -1, 1, 2, unit='m'))
+    detector.attrs['axes'] = ['xx', 'yy']
+    detector.attrs['x_pixel_offset_indices'] = [0]
+    detector.attrs['y_pixel_offset_indices'] = [1]
+    snx.create_field(
+        detector, 'depends_on', sc.scalar('/instrument/detector_0/transformations/t1')
+    )
+    transformations = snx.create_class(detector, 'transformations', NXtransformations)
+    value = sc.scalar(6.5, unit='mm')
+    offset = sc.spatial.translation(value=[1, 2, 3], unit='mm')
+    vector = sc.vector(value=[0, 0, 1])
+    t = value * vector
+    value1 = snx.create_field(transformations, 't1', value)
+    value1.attrs['depends_on'] = 't2'
+    value1.attrs['transformation_type'] = 'translation'
+    value1.attrs['offset'] = offset.values
+    value1.attrs['offset_units'] = str(offset.unit)
+    value1.attrs['vector'] = vector.value
+    value2 = snx.create_field(transformations, 't2', value.to(unit='cm'))
+    value2.attrs['depends_on'] = '/instrument/detector_0/transformations/t2'
+    value2.attrs['transformation_type'] = 'translation'
+    value2.attrs['vector'] = vector.value
+
+    t1 = sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit) * offset
+    t2 = sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit).to(
+        unit='cm'
+    )
+    root = make_group(h5root)
+    loaded = root[()]
+    result = snx.compute_positions(loaded)
+    origin = sc.vector([0, 0, 0], unit='m')
+    assert_identical(
+        result['instrument']['detector_0']['position'],
+        t2.to(unit='m') * t1.to(unit='m') * origin,
+    )
+    assert_identical(
+        result['instrument']['detector_0']['data'].coords['position'],
+        t2.to(unit='m')
+        * t1.to(unit='m')
+        * sc.vectors(
+            dims=['xx', 'yy'],
+            values=[[[-1, -1, 0], [-1, 1, 0]], [[1, -1, 0], [1, 1, 0]]],
+            unit='m',
+        ),
+    )
+
+
+def test_compute_positions_terminates_with_depends_on_to_self_relative(h5root):
+    instrument = snx.create_class(h5root, 'instrument', snx.NXinstrument)
+    detector = create_detector(instrument)
+    snx.create_field(detector, 'x_pixel_offset', sc.linspace('xx', -1, 1, 2, unit='m'))
+    snx.create_field(detector, 'y_pixel_offset', sc.linspace('yy', -1, 1, 2, unit='m'))
+    detector.attrs['axes'] = ['xx', 'yy']
+    detector.attrs['x_pixel_offset_indices'] = [0]
+    detector.attrs['y_pixel_offset_indices'] = [1]
+    snx.create_field(
+        detector, 'depends_on', sc.scalar('/instrument/detector_0/transformations/t1')
+    )
+    transformations = snx.create_class(detector, 'transformations', NXtransformations)
+    value = sc.scalar(6.5, unit='mm')
+    offset = sc.spatial.translation(value=[1, 2, 3], unit='mm')
+    vector = sc.vector(value=[0, 0, 1])
+    t = value * vector
+    value1 = snx.create_field(transformations, 't1', value)
+    value1.attrs['depends_on'] = 't2'
+    value1.attrs['transformation_type'] = 'translation'
+    value1.attrs['offset'] = offset.values
+    value1.attrs['offset_units'] = str(offset.unit)
+    value1.attrs['vector'] = vector.value
+    value2 = snx.create_field(transformations, 't2', value.to(unit='cm'))
+    value2.attrs['depends_on'] = 't2'
+    value2.attrs['transformation_type'] = 'translation'
+    value2.attrs['vector'] = vector.value
+
+    t1 = sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit) * offset
+    t2 = sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit).to(
+        unit='cm'
+    )
+    root = make_group(h5root)
+    loaded = root[()]
+    result = snx.compute_positions(loaded)
+    origin = sc.vector([0, 0, 0], unit='m')
+    assert_identical(
+        result['instrument']['detector_0']['position'],
+        t2.to(unit='m') * t1.to(unit='m') * origin,
+    )
+    assert_identical(
+        result['instrument']['detector_0']['data'].coords['position'],
+        t2.to(unit='m')
+        * t1.to(unit='m')
+        * sc.vectors(
+            dims=['xx', 'yy'],
+            values=[[[-1, -1, 0], [-1, 1, 0]], [[1, -1, 0], [1, 1, 0]]],
+            unit='m',
+        ),
+    )
+
+
+def test_compute_positions_raises_for_depends_on_cycle(h5root):
+    instrument = snx.create_class(h5root, 'instrument', snx.NXinstrument)
+    detector = create_detector(instrument)
+    snx.create_field(detector, 'x_pixel_offset', sc.linspace('xx', -1, 1, 2, unit='m'))
+    snx.create_field(detector, 'y_pixel_offset', sc.linspace('yy', -1, 1, 2, unit='m'))
+    detector.attrs['axes'] = ['xx', 'yy']
+    detector.attrs['x_pixel_offset_indices'] = [0]
+    detector.attrs['y_pixel_offset_indices'] = [1]
+    snx.create_field(
+        detector, 'depends_on', sc.scalar('/instrument/detector_0/transformations/t1')
+    )
+    transformations = snx.create_class(detector, 'transformations', NXtransformations)
+    value = sc.scalar(6.5, unit='mm')
+    offset = sc.spatial.translation(value=[1, 2, 3], unit='mm')
+    vector = sc.vector(value=[0, 0, 1])
+    value1 = snx.create_field(transformations, 't1', value)
+    value1.attrs['depends_on'] = 't2'
+    value1.attrs['transformation_type'] = 'translation'
+    value1.attrs['offset'] = offset.values
+    value1.attrs['offset_units'] = str(offset.unit)
+    value1.attrs['vector'] = vector.value
+    value2 = snx.create_field(transformations, 't2', value.to(unit='cm'))
+    value2.attrs['depends_on'] = 't1'
+    value2.attrs['transformation_type'] = 'translation'
+    value2.attrs['vector'] = vector.value
+
+    root = make_group(h5root)
+    with pytest.raises(ValueError, match="Circular depends_on"):
+        _ = root[()]