Repository navigation
[essreduce] Add failsafe loaders (generic, loki, dream) - #785
Conversation
SimonHeybrock
left a comment
There was a problem hiding this comment.
Having a loader for inspecting files that the reduction workflows cannot handle makes sense. Not computing positions when the transformation is time-dependent is fine for that purpose.
I ran both code paths on every detector and monitor in the 2026-10-06 coda drop (all ten instruments). 26 components fail in the workflow and use the fallback. 15 of these are caused by two essreduce bugs, not by broken files:
- #797: monitors written by the current file writer (
auxiliary_signals=['frame_total']) fail withKeyError: 'frame_total'. This affects DREAM, LoKI, NMX and BIFROST. It is also what the "badly formed monitor data" inloki_broken_fileis. - #798: monitors with
depends_on='.'fail with aUnitError. This affects all FREIA monitors and MAGICbeam_monitor_1.
I will open fixes for both. After that, the fallback is only used for files that are actually broken: empty NXlogs in the transformation chain, dangling depends_on chains, and the BEER detectors where ScippNeXus cannot determine the signal. In this drop, no monitor is returned as a raw DataGroup once these are fixed. Some tests in this PR expect the fallback warnings for monitors, so they will need updating (see inline).
Two remarks on the interface:
- Naming and placement.
ess.reduce.nexus.load_detectorssits next toload_dataandload_component, which feed the reduction.ess.loki.load_detectorsreads like the way to load LoKI data. Once people use these names, it is hard to change what they mean. Can we make the inspection purpose visible, e.g. aess.reduce.nexus.inspectmodule or a more specific name? - No way to select pulses.
load_detectors(filename)loads all events of all banks. The workflow already supportsTimeInterval. It does not need to be part of this PR, but the signature should leave room for it (keyword-only arguments afterfilename).
| try: | ||
| dg[detector] = _load_detector_with_workflow(wf, detector) | ||
| except Exception: # noqa: PERF203 | ||
| try: | ||
| dg[detector] = _load_detector_with_fallback(filename, detector) | ||
| except Exception: # noqa: S112 | ||
| continue |
There was a problem hiding this comment.
If both paths fail, the component is dropped without any message. Examples from current files:
load_detectors('does_not_exist.hdf', detectors=['loki_detector_0']) # DataGroup(keys=[])
load_monitors(loki_file, monitors=['typo_name']) # DataGroup(keys=[])
load_detectors(tbl_file) # 6 of 7 detectors, orca_detector missingFor a tool meant for debugging, this hides exactly what the user needs to know. Suggestion: raise if a name was requested explicitly, and warn and skip when loading everything (None).
Related: the warnings the tests check for ("depends_on chain", "Falling back") come from ScippNeXus, not from this code. Nothing tells the user that the fallback was used and why the result looks different. Something like this would help, and it gives the tests a warning of our own to match:
warnings.warn(f"{name}: workflow failed ({e}), loaded raw NeXus group instead", stacklevel=2)Catching Exception in both blocks also hides bugs in our own code, such as #797 and #798.
There was a problem hiding this comment.
Catching Exception in both blocks also hides bugs in our own code, such as #798.
What would be the alternative to catching Exception if we want the loaders to be fail-safe?
There was a problem hiding this comment.
See above:
For a tool meant for debugging, this hides exactly what the user needs to know. Suggestion: raise if a name was requested explicitly, and warn and skip when loading everything (None).
There was a problem hiding this comment.
Sure, I implemented that. I'm just wondering about the part where we don't want to hide bugs in our code. Should the actual error be brought up to the top level (either in the form of an error or a warning) so we can see it?
I think this will constitute unnecessary noise for the user?
As it currently stands, we get something like:
UserWarning: loki_detector_0: loading with default loader failed, attempting fallback
UserWarning: depends_on chain {x} contains empty time-series, transform = combine_transformations([t.build() for t in chain])
So we see what went wrong inside the fallback, but not the error that was raised by the workflow loader.
The info from the workflow might be useful for us? Should I add a flag to control how much we want printed?
There was a problem hiding this comment.
I think this is fine, we can iterate later if it is insufficient.
There was a problem hiding this comment.
One thing I missed earlier: the raise/warn for components that fail to load never runs. The continue in the fallback's except skips the if item not in dg check, so a requested name that does not exist still comes back silently:
load_monitors(loki_coda_file, monitors=['typo_name']) # DataGroup(keys=[]), no error
load_detectors('does_not_exist.hdf', detectors=['loki_detector_0']) # sameReplacing continue with pass should fix it. Could you add a test for this case, e.g. a non-existent name expecting RuntimeError?
| detectors: | ||
| List of detectors to load. A single string can also be provided to load | ||
| only one detector. If ``None``, all NXdetectors are loaded. | ||
| """ |
There was a problem hiding this comment.
Could the docstring say what the user gets back? The result depends on which path succeeded: position is only present when it could be computed, and a monitor that ScippNeXus cannot interpret comes back as a raw DataGroup. The instrument_view cell in the notebook relies on position, for example.
| return wf | ||
|
|
||
|
|
||
| def _find_detectors_or_monitors( |
There was a problem hiding this comment.
read_nexus_file_info in _nexus_loader.py already lists detectors and monitors. It finds the entry and instrument groups with _unique_child_group instead of a hardcoded /entry/instrument. Could we reuse it?
There was a problem hiding this comment.
I made use of read_nexus_file_info and _unique_child_group and removed the hardcoded /entry/instrument path.
| return wf.compute(RawMonitor[AnyRun, AnyMonitor]) | ||
|
|
||
|
|
||
| def _load_monitor_with_fallback(filename: str | Path, monitor: str) -> sc.DataArray: |
There was a problem hiding this comment.
This is identical to _load_detector_with_fallback, and load_monitors is a copy of load_detectors apart from the fold. One private function that takes the NeXus class and the workflow key would halve the module.
| if fold is not None: | ||
| for key, da in dg.items(): | ||
| if key in fold: | ||
| dg[key] = da.fold(dim=da.dim, sizes=fold[key]) |
There was a problem hiding this comment.
da.dim raises for data with more than one dimension. The BEER detectors already come back from the fallback with three unnamed dims (dim_0, dim_1, dim_2, 12×1000×1000), so a BEER wrapper would fail here.
Also, the workflow already folds via DetectorBankSizes, before computing positions. Setting wf[DetectorBankSizes] = bank_sizes would reuse that for the workflow path, and only the fallback would need folding here.
|
|
||
|
|
||
| def test_load_monitors_all_monitors(): | ||
| with pytest.warns(UserWarning, match="Failed to load"): |
There was a problem hiding this comment.
This warning comes from #797 (KeyError: 'frame_total' in the workflow, then the fallback). The test will fail once that is fixed. The same applies to the "Falling back" checks for monitors in the essreduce and esssans tests.
| from ess import loki | ||
| from ess.loki import data | ||
|
|
||
| FILES = [data.loki_coda_file(), data.loki_broken_file()] |
There was a problem hiding this comment.
This downloads both files at import time, i.e. during test collection. Without network access, collection of the whole test module fails rather than the individual tests. A fixture (session-scoped, like in essreduce's conftest.py) avoids that.
| "The LokiBankViewer can only be used with interactive figures" | ||
| "in a Jupyter notebook. Use `%matplotlib widget` at the start of " |
There was a problem hiding this comment.
Missing space: this renders as "interactive figuresin a Jupyter notebook".
| "source": [ | ||
| "tool = p.children[0].toolbar['inspect']\n", | ||
| "tool.value = True\n", | ||
| "tool._tool.click(27, 20)\n", |
There was a problem hiding this comment.
tool._tool.click(...) is private plopp API. A plopp change can break this docs page without warning. Is there a public way to do this, or could we show the inspector without pre-drawn rectangles?
There was a problem hiding this comment.
This is done in the same way in other places. In any cases, and change in Plopp's API would be beyond the scope of this PR.
or could we show the inspector without pre-drawn rectangles?
Not really, the plot would not be very interesting.
I moved things in a |
|
The module name However, I can see that |
…op, and add tests for bad file names and bad detector/monitor names
This PR adds some generic 'fail-safe' loaders
load_detectors()andload_monitors()to load data from files without failing if some transformations are incomplete or other common errors.The functions first try to use the
GenericNeXusWorkflowto load the data, but falls back to raw Scippnexus code if exceptions are raised.Some warnings may be raised, but something should still be loaded.
By default, all detector banks or monitors are loaded. You can pass a list of detector or monitor names to load if you want to load only a subset:
load_detectors(filename="file.hdf", detectors=["bank_0", "bank_1"]).Some instrument-specific wrappers have also been added for Loki and Dream, which reshape the loaded detector banks into logical dimensions:
ess.loki.diagnostics.load_detectors(),ess.loki.diagnostics.load_monitors().Roll-out to other instruments will be done in a follow-up.