Repository: scipp/scippnexus

Issue:
More selective fallback loader
Currently, the fallback loader in `NXobject` catches (nearly) all exceptions from loaders for concrete classes and uses the fallback. This is intended to allow loading files with partially bad structure. But it also hides user errors like a bad index (wrong dim, bad slice, etc.).

We should distinguish between errors originating in the file structure and error originating from the user/caller. Only the former should trigger the fallback.

Discussion:

Patch:
diff --git a/conda/meta.yaml b/conda/meta.yaml
index 9dab415..48f3fef 100644
--- a/conda/meta.yaml
+++ b/conda/meta.yaml
@@ -21,7 +21,6 @@ test:
   requires:
     - pytest
   source_files:
-    - pyproject.toml
     - tests/
   commands:
     - python -m pytest tests
diff --git a/src/scippnexus/_common.py b/src/scippnexus/_common.py
index 7a1d67e..9604076 100644
--- a/src/scippnexus/_common.py
+++ b/src/scippnexus/_common.py
@@ -56,8 +56,8 @@ def _to_canonical_select(dims: List[str],
 
     def check_1d():
         if len(dims) != 1:
-            raise ValueError(f"Dataset has multiple dimensions {dims}, "
-                             "specify the dimension to index.")
+            raise sc.DimensionError(f"Dataset has multiple dimensions {dims}, "
+                                    "specify the dimension to index.")
 
     if select is Ellipsis:
         return {}
@@ -69,14 +69,14 @@ def _to_canonical_select(dims: List[str],
     if isinstance(select, tuple):
         check_1d()
         if len(select) != 1:
-            raise ValueError(f"Dataset has single dimension {dims}, "
-                             "but multiple indices {select} were specified.")
+            raise sc.DimensionError(f"Dataset has single dimension {dims}, "
+                                    "but multiple indices {select} were specified.")
         return {dims[0]: select[0]}
     elif isinstance(select, int) or isinstance(select, slice):
         check_1d()
         return {dims[0]: select}
     if not isinstance(select, dict):
-        raise ValueError(f"Cannot process index {select}.")
+        raise IndexError(f"Cannot process index {select}.")
     return select
 
 
@@ -88,7 +88,7 @@ def to_plain_index(dims: List[str], select: ScippIndex) -> Union[int, slice, tup
     index = [slice(None)] * len(dims)
     for key, sel in select.items():
         if key not in dims:
-            raise ValueError(
+            raise sc.DimensionError(
                 f"'{key}' used for indexing not found in dataset dims {dims}.")
         index[dims.index(key)] = sel
     if len(index) == 1:
diff --git a/src/scippnexus/nxdata.py b/src/scippnexus/nxdata.py
index 4a59144..ce7071d 100644
--- a/src/scippnexus/nxdata.py
+++ b/src/scippnexus/nxdata.py
@@ -137,6 +137,9 @@ class NXdata(NXobject):
         if self._signal_override is not None:
             return self._signal_override
         if self._signal_name is not None:
+            if self._signal_name not in self:
+                raise NexusStructureError(
+                    f"Signal field '{self._signal_name}' not found in group.")
             return self[self._signal_name]
         return None
 
@@ -225,7 +228,7 @@ class NXdata(NXobject):
         from .nexus_classes import NXgeometry
         signal = self._signal
         if signal is None:
-            raise NexusStructureError("No signal field found, cannot load group")
+            raise NexusStructureError("No signal field found, cannot load group.")
         signal = signal[select]
         if self._errors_name is not None:
             stddevs = self[self._errors_name][select]
@@ -266,11 +269,16 @@ class NXdata(NXobject):
             if (error_name := self._strategy.coord_errors(self, name)) is not None:
                 stddevs = asarray(self[error_name][sel])
                 coord.variances = sc.pow(stddevs, sc.scalar(2)).values
-            if self._coord_to_attr(da, name, field):
-                # Like scipp, slicing turns coord into attr if slicing removes the
-                # dim corresponding to the coord.
-                da.attrs[name] = coord
-            else:
-                da.coords[name] = coord
+            try:
+                if self._coord_to_attr(da, name, field):
+                    # Like scipp, slicing turns coord into attr if slicing removes the
+                    # dim corresponding to the coord.
+                    da.attrs[name] = coord
+                else:
+                    da.coords[name] = coord
+            except sc.DimensionError as e:
+                raise NexusStructureError(
+                    f"Field in NXdata incompatible with dims or shape of signal: {e}"
+                ) from e
 
         return da
diff --git a/src/scippnexus/nxdetector.py b/src/scippnexus/nxdetector.py
index a2567c5..f0b00bc 100644
--- a/src/scippnexus/nxdetector.py
+++ b/src/scippnexus/nxdetector.py
@@ -99,7 +99,9 @@ class _EventField:
         return self._nxevent_data.unit
 
     def __getitem__(self, select: ScippIndex) -> sc.DataArray:
-        event_data: sc.DataArray = self._nxevent_data[self._event_select]
+        event_data = self._nxevent_data[self._event_select]
+        if isinstance(event_data, sc.DataGroup):
+            raise NexusStructureError("Invalid NXevent_data in NXdetector.")
         if self._grouping is None:
             if select not in (Ellipsis, tuple(), slice(None)):
                 raise NexusStructureError(
diff --git a/src/scippnexus/nxevent_data.py b/src/scippnexus/nxevent_data.py
index 59155cb..2351770 100644
--- a/src/scippnexus/nxevent_data.py
+++ b/src/scippnexus/nxevent_data.py
@@ -121,7 +121,7 @@ class NXevent_data(NXobject):
         try:
             binned = sc.bins(data=events, dim=_event_dimension, begin=begins, end=ends)
         except IndexError as e:
-            raise IndexError(
+            raise NexusStructureError(
                 f"Invalid index in NXevent_data at {self.name}/event_index:\n{e}.")
 
         return sc.DataArray(data=binned, coords={'event_time_zero': event_time_zero})
diff --git a/src/scippnexus/nxmonitor.py b/src/scippnexus/nxmonitor.py
index defbbc8..3061e8f 100644
--- a/src/scippnexus/nxmonitor.py
+++ b/src/scippnexus/nxmonitor.py
@@ -17,5 +17,5 @@ class NXmonitor(NXdetector):
         # binning present in the file (in NXevent_data) is preserved.
         return {
             'grouping_key': 'event_time_zero',
-            'grouping': self.events['event_time_zero']
+            'grouping': self.events.get('event_time_zero')
         }
diff --git a/src/scippnexus/nxobject.py b/src/scippnexus/nxobject.py
index f92eef8..5eb56f1 100644
--- a/src/scippnexus/nxobject.py
+++ b/src/scippnexus/nxobject.py
@@ -394,14 +394,14 @@ class NXobject:
         try:
             da = self._getitem(name)
             self._insert_leaf_properties(da)
-        except Exception as e:
+        except NexusStructureError as e:
             # If the child class cannot load this group, we fall back to returning the
             # underlying datasets in a DataGroup.
             if type(self)._getitem == NXobject._getitem:
                 raise
             else:
                 msg = (
-                    f"Failed to load {self.name} as {type(self).__name__}: {e}. "
+                    f"Failed to load {self.name} as {type(self).__name__}: {e} "
                     "Falling back to loading HDF5 group children as scipp.DataGroup.")
                 warnings.warn(msg)
             da = NXobject._getitem(self, name)
diff --git a/src/scippnexus/nxtransformations.py b/src/scippnexus/nxtransformations.py
index 30bc8b5..e6f666d 100644
--- a/src/scippnexus/nxtransformations.py
+++ b/src/scippnexus/nxtransformations.py
@@ -9,10 +9,10 @@ import numpy as np
 import scipp as sc
 from scipp.scipy import interpolate
 
-from .nxobject import Field, NXobject, ScippIndex
+from .nxobject import Field, NexusStructureError, NXobject, ScippIndex
 
 
-class TransformationError(Exception):
+class TransformationError(NexusStructureError):
     pass
 
 
@@ -74,28 +74,33 @@ class Transformation:
         # shape=[1] for single values. It is unclear how and if this could be
         # distinguished from a scan of length 1.
         value = self._obj[select]
-        if isinstance(value, sc.DataGroup):
-            raise TransformationError(f"Failed to load transformation at {self.name}.")
-        t = value * self.vector
-        v = t if isinstance(t, sc.Variable) else t.data
-        if transformation_type == 'translation':
-            v = v.to(unit='m', copy=False)
-            v = sc.spatial.translations(dims=v.dims, values=v.values, unit=v.unit)
-        elif transformation_type == 'rotation':
-            v = sc.spatial.rotations_from_rotvecs(v)
-        else:
-            raise TransformationError(
-                f"{transformation_type=} attribute at {self.name},"
-                " expected 'translation' or 'rotation'.")
-        if isinstance(t, sc.Variable):
-            t = v
-        else:
-            t.data = v
-        if (offset := self.offset) is None:
-            return t
-        offset = sc.vector(value=offset.values, unit=offset.unit).to(unit='m')
-        offset = sc.spatial.translation(value=offset.value, unit=offset.unit)
-        return t * offset
+        try:
+            if isinstance(value, sc.DataGroup):
+                raise TransformationError(
+                    f"Failed to load transformation at {self.name}.")
+            t = value * self.vector
+            v = t if isinstance(t, sc.Variable) else t.data
+            if transformation_type == 'translation':
+                v = v.to(unit='m', copy=False)
+                v = sc.spatial.translations(dims=v.dims, values=v.values, unit=v.unit)
+            elif transformation_type == 'rotation':
+                v = sc.spatial.rotations_from_rotvecs(v)
+            else:
+                raise TransformationError(
+                    f"{transformation_type=} attribute at {self.name},"
+                    " expected 'translation' or 'rotation'.")
+            if isinstance(t, sc.Variable):
+                t = v
+            else:
+                t.data = v
+            if (offset := self.offset) is None:
+                return t
+            offset = sc.vector(value=offset.values, unit=offset.unit).to(unit='m')
+            offset = sc.spatial.translation(value=offset.value, unit=offset.unit)
+            return t * offset
+        except (sc.DimensionError, sc.UnitError) as e:
+            raise NexusStructureError(
+                f"Invalid transformation in NXtransformations: {e}") from e
 
 
 def _interpolate_transform(transform, xnew):


Test patch:
diff --git a/tests/nexus_test.py b/tests/nexus_test.py
index 937f3bf..5ae9743 100644
--- a/tests/nexus_test.py
+++ b/tests/nexus_test.py
@@ -97,6 +97,15 @@ def test_nxobject_log(nxroot):
     assert sc.identical(log[...], da)
 
 
+def test_nxlog_with_missing_value_triggers_fallback(nxroot):
+    time = sc.epoch(unit='ns') + sc.array(
+        dims=['time'], unit='s', values=[4.4, 5.5, 6.6]).to(unit='ns', dtype='int64')
+    log = nxroot['entry'].create_class('log', NXlog)
+    log['time'] = time - sc.epoch(unit='ns')
+    loaded = log[()]
+    assert sc.identical(loaded, sc.DataGroup(time=time.rename(time='dim_0')))
+
+
 def test_nxlog_length_1(nxroot):
     da = sc.DataArray(
         sc.array(dims=['time'], values=[1.1]),
diff --git a/tests/nxdata_test.py b/tests/nxdata_test.py
index f7782d6..924476d 100644
--- a/tests/nxdata_test.py
+++ b/tests/nxdata_test.py
@@ -465,3 +465,16 @@ def test_nested_groups_trigger_fallback_to_load_as_data_group(nxroot):
     data.attrs['signal'] = 'signal'
     data.create_class('nested', NXdata)
     assert sc.identical(data[...], sc.DataGroup(signal=da.data, nested=sc.DataGroup()))
+
+
+def test_slicing_raises_given_invalid_index(nxroot):
+    signal = sc.array(dims=['xx', 'yy'], unit='m', values=[[1.1, 2.2], [3.3, 4.4]])
+    data = nxroot.create_class('data1', NXdata)
+    data.create_field('signal', signal)
+    data.attrs['axes'] = signal.dims
+    data.attrs['signal'] = 'signal'
+    assert sc.identical(data[...], sc.DataArray(signal))
+    with pytest.raises(IndexError):
+        data['xx', 2]
+    with pytest.raises(sc.DimensionError):
+        data['zz', 0]
diff --git a/tests/nxdetector_test.py b/tests/nxdetector_test.py
index 1f18afe..a667f59 100644
--- a/tests/nxdetector_test.py
+++ b/tests/nxdetector_test.py
@@ -265,7 +265,7 @@ def test_loading_event_data_with_selection_and_automatic_detector_numbers_raises
     detector = nxroot.create_class('detector0', NXdetector)
     create_event_data_ids_1234(detector.create_class('events', NXevent_data))
     assert detector.dims == ['detector_number']
-    with pytest.raises(ValueError):
+    with pytest.raises(sc.DimensionError):
         detector['detector_number', 0]