Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect cycles in depends_on #254

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Detect cycles in depends_on #254

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
517e6aa
Support self-references in depends_on
jl-wynen Nov 25, 2024
bd6d803
Detect cycles in depends_on
jl-wynen Nov 25, 2024
ed03e50
Include last node in error message
jl-wynen Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/scippnexus/field.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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):
Expand Down
9 changes: 9 additions & 0 deletions src/scippnexus/nxtransformations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,13 +308,22 @@ 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(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}'),
Expand Down
133 changes: 133 additions & 0 deletions tests/nxtransformations_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[()]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Did you try loading a NMX file from coda to check that the error is raised?

Copy link
Member Author

@jl-wynen jl-wynen Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did now and it does find a cycle:

'/entry/instrument/detector_panel_0/transformations/orientation',
'/entry/instrument/detector_panel_0/transformations/axis6',
'/entry/instrument/detector_panel_0/transformations/axis5',
'/entry/instrument/detector_panel_0/transformations/axis4',
'/entry/instrument/detector_panel_0/transformations/axis3',
'/entry/instrument/detector_panel_0/transformations/axis2',
'/entry/instrument/detector_panel_0/transformations/axis1'
'/entry/instrument/detector_panel_0/transformations/stageZ'
'/entry/instrument/detector_panel_0/transformations/orientation'