From 517e6aa4aabf6b25d642a610af24963c976b37dd Mon Sep 17 00:00:00 2001 From: Jan-Lukas Wynen Date: Mon, 25 Nov 2024 13:50:36 +0100 Subject: [PATCH 1/3] Support self-references in depends_on --- src/scippnexus/field.py | 11 ++- src/scippnexus/nxtransformations.py | 8 +++ tests/nxtransformations_test.py | 102 ++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 2 deletions(-) diff --git a/src/scippnexus/field.py b/src/scippnexus/field.py index d0e65d6d..be9dcdb3 100644 --- a/src/scippnexus/field.py +++ b/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 @@ -41,6 +43,11 @@ def absolute_path(self) -> str | None: return None return posixpath.normpath(posixpath.join(self.parent, self.value)) + def __eq__(self, other: object) -> bool: + if not isinstance(other, DependsOn): + return NotImplemented + return self.absolute_path() == other.absolute_path() + def _is_time(obj): if (unit := obj.unit) is None: @@ -113,7 +120,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 +146,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): diff --git a/src/scippnexus/nxtransformations.py b/src/scippnexus/nxtransformations.py index ff0cc6a4..6c648bee 100644 --- a/src/scippnexus/nxtransformations.py +++ b/src/scippnexus/nxtransformations.py @@ -308,6 +308,7 @@ 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] try: while depends_on.value != '.': transform, base = _locate_depends_on_target( @@ -315,6 +316,13 @@ def parse_depends_on_chain( ) depends_on = DependsOn(parent=base, value=transform.attrs['depends_on']) chain.transformations[transform.name] = transform[()] + if depends_on == 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 + + visited.append(depends_on) except KeyError as e: warnings.warn( UserWarning(f'depends_on chain {depends_on} references missing node {e}'), diff --git a/tests/nxtransformations_test.py b/tests/nxtransformations_test.py index 5f5a7d18..b99b7c4c 100644 --- a/tests/nxtransformations_test.py +++ b/tests/nxtransformations_test.py @@ -829,3 +829,105 @@ 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', + ), + ) From bd6d80316645974e6a9917f6abe13f60a5227cea Mon Sep 17 00:00:00 2001 From: Jan-Lukas Wynen Date: Mon, 25 Nov 2024 13:54:49 +0100 Subject: [PATCH 2/3] Detect cycles in depends_on --- src/scippnexus/field.py | 5 ----- src/scippnexus/nxtransformations.py | 9 +++++---- tests/nxtransformations_test.py | 31 +++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/scippnexus/field.py b/src/scippnexus/field.py index be9dcdb3..b7e60721 100644 --- a/src/scippnexus/field.py +++ b/src/scippnexus/field.py @@ -43,11 +43,6 @@ def absolute_path(self) -> str | None: return None return posixpath.normpath(posixpath.join(self.parent, self.value)) - def __eq__(self, other: object) -> bool: - if not isinstance(other, DependsOn): - return NotImplemented - return self.absolute_path() == other.absolute_path() - def _is_time(obj): if (unit := obj.unit) is None: diff --git a/src/scippnexus/nxtransformations.py b/src/scippnexus/nxtransformations.py index 6c648bee..4b8d59f3 100644 --- a/src/scippnexus/nxtransformations.py +++ b/src/scippnexus/nxtransformations.py @@ -308,7 +308,7 @@ 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] + visited = [depends_on.absolute_path()] try: while depends_on.value != '.': transform, base = _locate_depends_on_target( @@ -316,13 +316,14 @@ def parse_depends_on_chain( ) depends_on = DependsOn(parent=base, value=transform.attrs['depends_on']) chain.transformations[transform.name] = transform[()] - if depends_on == visited[-1]: + 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 - - visited.append(depends_on) + elif depends_on.absolute_path() in visited: + raise ValueError(f'Circular depends_on chain detected: {visited}') + visited.append(depends_on.absolute_path()) except KeyError as e: warnings.warn( UserWarning(f'depends_on chain {depends_on} references missing node {e}'), diff --git a/tests/nxtransformations_test.py b/tests/nxtransformations_test.py index b99b7c4c..b3611d0a 100644 --- a/tests/nxtransformations_test.py +++ b/tests/nxtransformations_test.py @@ -931,3 +931,34 @@ def test_compute_positions_terminates_with_depends_on_to_self_relative(h5root): 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[()] From ed03e50eb968cb79292f0a18fd46016c4636c601 Mon Sep 17 00:00:00 2001 From: Jan-Lukas Wynen Date: Mon, 25 Nov 2024 15:11:01 +0100 Subject: [PATCH 3/3] Include last node in error message --- src/scippnexus/nxtransformations.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/scippnexus/nxtransformations.py b/src/scippnexus/nxtransformations.py index 4b8d59f3..dde03cc3 100644 --- a/src/scippnexus/nxtransformations.py +++ b/src/scippnexus/nxtransformations.py @@ -322,7 +322,10 @@ def parse_depends_on_chain( # detect this case. chain.transformations[transform.name].depends_on = depends_on elif depends_on.absolute_path() in visited: - raise ValueError(f'Circular depends_on chain detected: {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(