Load NXdata auxiliary signals with distinct dimensions - #306
CAOShurong wants to merge 1 commit into
Conversation
Keep NXdata valid when an auxiliary signal uses dimensions unrelated to the primary signal, and fall back from Dataset to DataGroup while preserving assembled signal semantics. Add a focused regression for the ESS frame-total layout. Developed with OpenAI Codex assistance.
SimonHeybrock
left a comment
SimonHeybrock
left a comment
There was a problem hiding this comment.
Thanks for picking this up, and for the thorough validation notes.
However, I think we should probably close this PR for now. The main reason is that #304 has not settled on an approach yet. The issue lists three options, and the discussion so far leans towards not changing the default loader at all: filter auxiliary signals with an application definition instead, defined downstream. essreduce already does this as a workaround (it drops frame_total and reference_time via a custom NXdata definition), so there is no urgent need for a change here.
The approach in this PR also has a drawback for users. The return type of loading an NXdata now depends on the shapes in the file: a Dataset if the auxiliary signals are compatible with the signal, a DataGroup otherwise. Code consuming the result then has to handle both. Maybe we should always returns a DataGroup, similar to what we decided to do for some other classes that had similar issues?
Apart from the design question, I found a few correctness problems, see the inline comments. If we decide on the DataGroup route in the issue, these would need to be addressed. We would be happy to continue there.
| # len(dims). | ||
| shape = _squeeze_trailing(dims, field.dataset.shape) | ||
| field.sizes = dict(zip(dims, shape, strict=False)) | ||
| elif name in self._aux_signals: |
There was a problem hiding this comment.
Skipping the consistency check means the auxiliary signal keeps a generated dim label such as dim_0, and NXdata.sizes merges these into the group sizes. This can silently collide with the signal's dims. Example: signal with axes=['.'] (so its dim is dim_0, length 3) and frame_total with length 2:
group.sizesreports{'dim_0': 2}group['dim_0', 2]raisesIndexError, although it is a valid index for the signalgroup['dim_0', 1]slices signal andframe_totalwith the same index, returning wrong data
On main this file is rejected with a fallback warning, which is the safer outcome.
| return sc.Dataset(signals) | ||
| try: | ||
| return sc.Dataset(signals) | ||
| except sc.DimensionError: |
There was a problem hiding this comment.
Catching DimensionError here also hides genuine inconsistencies that are unrelated to "auxiliary signal on different dims", for example *_indices attributes that assign the same dim label with a different length. Previously these produced the fallback warning, now they load silently as a DataGroup.
| assert_identical(data[...], sc.Dataset({'signal': signal, 'xx': aux})) | ||
|
|
||
|
|
||
| def test_auxiliary_signal_with_different_dims_loads_as_data_group(h5root) -> None: |
There was a problem hiding this comment.
This covers a bare NXdata with only frame_total. Two cases from the issue are not covered:
- The group also contains a non-signal field on the frame dimension, such as
reference_time(essreduce drops it for the same reason). With this PR such a group is still marked invalid and falls back with a warning, so the ESS monitor case would likely still fail. - An
NXmonitor, which goes through_assemble_as_physical_componentrather than_assemble_as_data.
This branch has not been deployed
2 participants
Summary
NXdatavalid when an auxiliary signal has dimensions unrelated to the primary signalscipp.DataGroupwhen those signals cannot share ascipp.DatasetDatasetresult for dimension-compatible auxiliary signalsWhy
NeXus permits auxiliary signals such as ESS
frame_totalto use dimensions that are unrelated to the primary signal. ScippNexus currently marks the wholeNXdatainvalid when it cannot infer the auxiliary signal's dimensions from the primary signal. Loading then emits a fallback warning and returns raw fields, so the primary signal also loses its assembledDataArraysemantics.The new path keeps the auxiliary field's fallback dimensions and only falls back from
DatasettoDataGroupwhen Scipp rejects the mixed dimensionality. The primary signal is still assembled as aDataArray, including its coordinates, while auxiliary signals remain independently accessible.Fixes #304.
Validation
mainwith the fallback warning and a rawVariableprimary signalNXdatamodule: 89 passedeaf7f84aDataGroup, compatible signals remain aDataset, no warning is emitted, andpip checkpassesThis pull request was developed with OpenAI Codex assistance; I reviewed the implementation and validation results.