Skip to content

[Fix] depends_on targets can be (NXcoordinate_system) groups#297

Open
g5t wants to merge 1 commit intoscipp:mainfrom
g5t:depends-on-group
Open

[Fix] depends_on targets can be (NXcoordinate_system) groups#297
g5t wants to merge 1 commit intoscipp:mainfrom
g5t:depends-on-group

Conversation

@g5t
Copy link
Contributor

@g5t g5t commented Feb 9, 2026

The @depends_on attribute of an entry in a NXtransformations group can be either another entry in an NXtransformations group or a NXcoordinate_system group:

@depends_on: (optional) [NX_CHAR](https://manual.nexusformat.org/nxdl-types.html#nx-char)

    Points to the path of a field defining the axis on which this instance of ...

    Points to the path of a field defining the axis on which this instance of NXtransformations depends or the string “.”. It can also point to an instance of NX_coordinate_system if this transformation depends on it.

    If it is the string “.”, it is explicitly pointing towards the default [NeXus coordinate system](https://manual.nexusformat.org/design.html#the-nexus-coordinate-system).

The NXcoordinate_system group has a depends_on entry not a @depends_on attribute.

This change allows for the target of a transformation's @depends_on attribute to be either a group (with a depends_on entry) or a dataset (with a @depends_on attribute).

Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

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

@SimonHeybrock can you take a look? I think we should extend this at least to NXcoordinate_system since the standard allows it. But I would prefer to not allow arbitrary classes in depends on chains.

Comment on lines +310 to +311
if isinstance(parent, Group) and name in parent:
target = parent[name][...]
Copy link
Member

Choose a reason for hiding this comment

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

Minor optimisation, probably irrelevant:

Suggested change
if isinstance(parent, Group) and name in parent:
target = parent[name][...]
if isinstance(parent, Group) and (target := ds.get(name)) is not None:
target = ds[...]

@SimonHeybrock
Copy link
Member

@SimonHeybrock can you take a look? I think we should extend this at least to NXcoordinate_system since the standard allows it. But I would prefer to not allow arbitrary classes in depends on chains.

Yes, this should check the NX_class attribute. But even if we do that, this change might not be sufficient, because we are not supporting NXcoordinate_system yet? @g5t is this something you need, or is just having the chain (with potentially broken chain links?) enough?

@g5t
Copy link
Contributor Author

g5t commented Feb 11, 2026

I'm only using NXcoordinate_system for chaining the transformations at the moment.

My current NeXus structure abuses this implementation to chain through a NXcrystal; and while I like keeping this possibility, I could refactor my implementation to more-strictly comply with the standard.

@SimonHeybrock
Copy link
Member

I'm only using NXcoordinate_system for chaining the transformations at the moment.

Are you implying that you don't actually want the NXcoordinate_system represented in the resolved chain, but simply "foward" to the chain within the coord-system?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants