Repository: scipp/scippnexus

Issue:
Improve loading of NXtransformations
Currently, when there is a `depend_on` in, e.g., an `NXdetector`, the corresponding transformation is loaded as a Scipp `affine_transform3`. This is fine. However, if there is no `depends_on`, `NXtransformations` are just loaded as their datasets. The problem is that vital information for transformations is stored in their attributes. This is cumbersome (hard to use) and currently not supported by `scipp.Variable` or `scipp.DataGroup`.

Instead, we should load the transformations in `NXtransformations` as `scipp.Variable` with the correct spatial dtype (could be, e.g., `rotation3` or `translation3`). The code for this all exists since it is used for computing the `depends_on` transformation chain, but it is not called when loading a plain `NXtransformations` group.

Discussion:

Patch:
diff --git a/src/scippnexus/nexus_classes.py b/src/scippnexus/nexus_classes.py
index 171a54c..7fc5080 100644
--- a/src/scippnexus/nexus_classes.py
+++ b/src/scippnexus/nexus_classes.py
@@ -13,6 +13,7 @@ from .nxobject import NXobject, NXroot  # noqa F401
 from .nxoff_geometry import NXoff_geometry  # noqa F401
 from .nxsample import NXsample  # noqa F401
 from .nxsource import NXsource  # noqa F401
+from .nxtransformations import NXtransformations  # noqa F401
 
 
 class NXentry(NXobject):
@@ -23,10 +24,6 @@ class NXinstrument(NXobject):
     """Group of instrument-related information."""
 
 
-class NXtransformations(NXobject):
-    """Group of transformations."""
-
-
 class NXaperture(NXobject):
     """NXaperture"""
 
diff --git a/src/scippnexus/nxdetector.py b/src/scippnexus/nxdetector.py
index fb1146e..6c21ac4 100644
--- a/src/scippnexus/nxdetector.py
+++ b/src/scippnexus/nxdetector.py
@@ -4,7 +4,7 @@
 from __future__ import annotations
 
 from copy import copy
-from typing import List, Optional, Union
+from typing import Any, Dict, List, Optional, Union
 
 import scipp as sc
 
@@ -91,8 +91,8 @@ class _EventField:
         return self._grouping.shape
 
     @property
-    def unit(self):
-        self._nxevent_data.unit
+    def unit(self) -> None:
+        return self._nxevent_data.unit
 
     def __getitem__(self, select: ScippIndex) -> sc.DataArray:
         event_data: sc.DataArray = self._nxevent_data[self._event_select]
@@ -164,7 +164,7 @@ class NXdetector(NXobject):
         return self._signal.unit
 
     @property
-    def _event_grouping(self) -> Union[None, Field]:
+    def _event_grouping(self) -> Union[None, Dict[str, Any]]:
         for key in self._detector_number_fields:
             if key in self:
                 return {'grouping_key': key, 'grouping': self[key]}
diff --git a/src/scippnexus/nxtransformations.py b/src/scippnexus/nxtransformations.py
index 48899ea..30bc8b5 100644
--- a/src/scippnexus/nxtransformations.py
+++ b/src/scippnexus/nxtransformations.py
@@ -1,16 +1,13 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright (c) 2023 Scipp contributors (https://github.com/scipp)
 # @author Simon Heybrock
-from typing import Union
+from __future__ import annotations
+
+from typing import List, Optional, Union
 
 import numpy as np
 import scipp as sc
-import scipp.spatial
-
-try:
-    from scipp.scipy import interpolate
-except ImportError:  # scipp<22.11
-    from scipp import interpolate
+from scipp.scipy import interpolate
 
 from .nxobject import Field, NXobject, ScippIndex
 
@@ -19,7 +16,7 @@ class TransformationError(Exception):
     pass
 
 
-def make_transformation(obj, /, path):
+def make_transformation(obj, /, path) -> Optional[Transformation]:
     if path.startswith('/'):
         return Transformation(obj.file[path])
     elif path != '.':
@@ -27,6 +24,17 @@ def make_transformation(obj, /, path):
     return None  # end of chain
 
 
+class NXtransformations(NXobject):
+    """Group of transformations."""
+
+    def _getitem(self, index: ScippIndex) -> sc.DataGroup:
+        return sc.DataGroup({
+            name: get_full_transformation_starting_at(Transformation(child),
+                                                      index=index)
+            for name, child in self.items()
+        })
+
+
 class Transformation:
 
     def __init__(self, obj: Union[Field, NXobject]):  # could be an NXlog
@@ -110,7 +118,8 @@ def _smaller_unit(a, b):
         return b.unit
 
 
-def get_full_transformation(depends_on: Field) -> Union[None, sc.DataArray]:
+def get_full_transformation(
+        depends_on: Field) -> Union[None, sc.DataArray, sc.Variable]:
     """
     Get the 4x4 transformation matrix for a component, resulting
     from the full chain of transformations linked by "depends_on"
@@ -118,12 +127,20 @@ def get_full_transformation(depends_on: Field) -> Union[None, sc.DataArray]:
     """
     if (t0 := make_transformation(depends_on, depends_on[()])) is None:
         return None
-    transformations = _get_transformations(t0)
+    return get_full_transformation_starting_at(t0)
+
 
-    total_transform = sc.spatial.affine_transform(value=np.identity(4), unit=sc.units.m)
+def get_full_transformation_starting_at(
+        t0: Transformation,
+        *,
+        index: ScippIndex = None) -> Union[None, sc.DataArray, sc.Variable]:
+    transformations = _get_transformations(t0, index=() if index is None else index)
 
+    total_transform = None
     for transform in transformations:
-        if isinstance(total_transform, sc.DataArray) and isinstance(
+        if total_transform is None:
+            total_transform = transform
+        elif isinstance(total_transform, sc.DataArray) and isinstance(
                 transform, sc.DataArray):
             unit = _smaller_unit(transform.coords['time'],
                                  total_transform.coords['time'])
@@ -146,12 +163,13 @@ def get_full_transformation(depends_on: Field) -> Union[None, sc.DataArray]:
     return total_transform
 
 
-def _get_transformations(transform: Union[Field, NXobject]):
+def _get_transformations(transform: Transformation, *,
+                         index: ScippIndex) -> List[Union[sc.DataArray, sc.Variable]]:
     """Get all transformations in the depends_on chain."""
     transformations = []
     t = transform
     while t is not None:
-        transformations.append(t[()])
+        transformations.append(t[index])
         t = t.depends_on
     # TODO: this list of transformation should probably be cached in the future
     # to deal with changing beamline components (e.g. pixel positions) during a


Test patch:
diff --git a/tests/nxtransformations_test.py b/tests/nxtransformations_test.py
index 649f085..0fcdaba 100644
--- a/tests/nxtransformations_test.py
+++ b/tests/nxtransformations_test.py
@@ -14,7 +14,7 @@ from scippnexus import (
 
 
 @pytest.fixture()
-def nxroot(request):
+def nxroot():
     """Yield NXroot containing a single NXentry named 'entry'"""
     with h5py.File('dummy.nxs', mode='w', driver="core", backing_store=False) as f:
         root = NXroot(f)
@@ -77,10 +77,8 @@ def test_chain_with_single_values_and_different_unit(nxroot):
     value2.attrs['transformation_type'] = 'translation'
     value2.attrs['vector'] = vector.value
 
-    expected = sc.spatial.affine_transform(value=np.identity(4), unit=t.unit)
-    expected = expected * sc.spatial.translations(
-        dims=t.dims, values=2 * t.values, unit=t.unit)
-    expected = expected * sc.spatial.translation(value=[0.001, 0.002, 0.003], unit='m')
+    expected = (sc.spatial.translations(dims=t.dims, values=2 * t.values, unit=t.unit) *
+                sc.spatial.translation(value=[0.001, 0.002, 0.003], unit='m'))
     assert sc.identical(detector[...].coords['depends_on'], expected)
 
 
@@ -141,8 +139,7 @@ def test_chain_with_multiple_values(nxroot):
     value2.attrs['transformation_type'] = 'translation'
     value2.attrs['vector'] = vector.value
 
-    expected = sc.spatial.affine_transform(value=np.identity(4), unit=t.unit)
-    expected = t * (t * (offset * expected))
+    expected = t * (t * offset)
     assert sc.identical(detector[...].coords['depends_on'].value, expected)
 
 
@@ -175,8 +172,7 @@ def test_chain_with_multiple_values_and_different_time_unit(nxroot):
     value2.attrs['transformation_type'] = 'translation'
     value2.attrs['vector'] = vector.value
 
-    expected = sc.spatial.affine_transform(value=np.identity(4), unit=t.unit)
-    expected = t * (t * (offset * expected))
+    expected = t * (t * offset)
     assert sc.identical(detector[...].coords['depends_on'].value, expected)
 
 
@@ -211,3 +207,100 @@ def test_broken_time_dependent_transformation_returns_path_and_transformations(n
     # attributes. DataGroup does currently not support attributes, so this information
     # is mostly useless until that is addressed.
     assert 't1' in t
+
+
+def write_translation(group, name: str, value: sc.Variable, offset: sc.Variable,
+                      vector: sc.Variable) -> None:
+    dset = group.create_field(name, value)
+    dset.attrs['transformation_type'] = 'translation'
+    dset.attrs['offset'] = offset.values
+    dset.attrs['offset_units'] = str(offset.unit)
+    dset.attrs['vector'] = vector.value
+
+
+def test_nxtransformations_group_single_item(nxroot):
+    value = sc.scalar(2.4, unit='mm')
+    offset = sc.spatial.translation(value=[6, 2, 6], unit='mm')
+    vector = sc.vector(value=[0, 1, 1])
+    t = value.to(unit='m') * vector
+    expected = (sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit) *
+                sc.spatial.translation(value=[0.006, 0.002, 0.006], unit='m'))
+
+    transformations = nxroot.create_class('transformations', NXtransformations)
+    write_translation(transformations, 't1', value, offset, vector)
+
+    loaded = nxroot['transformations'][()]
+    assert set(loaded.keys()) == {'t1'}
+    assert sc.identical(loaded['t1'], expected)
+
+
+def test_nxtransformations_group_two_independent_items(nxroot):
+    transformations = nxroot.create_class('transformations', NXtransformations)
+
+    value = sc.scalar(2.4, unit='mm')
+    offset = sc.spatial.translation(value=[6, 2, 6], unit='mm')
+    vector = sc.vector(value=[0, 1, 1])
+    t = value.to(unit='m') * vector
+    write_translation(transformations, 't1', value, offset, vector)
+    expected1 = (sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit) *
+                 sc.spatial.translation(value=[0.006, 0.002, 0.006], unit='m'))
+
+    value = value * 0.1
+    t = value.to(unit='m') * vector
+    write_translation(transformations, 't2', value, offset, vector)
+    expected2 = (sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit) *
+                 sc.spatial.translation(value=[0.006, 0.002, 0.006], unit='m'))
+
+    loaded = nxroot['transformations'][()]
+    assert set(loaded.keys()) == {'t1', 't2'}
+    assert sc.identical(loaded['t1'], expected1)
+    assert sc.identical(loaded['t2'], expected2)
+
+
+def test_nxtransformations_group_single_chain(nxroot):
+    transformations = nxroot.create_class('transformations', NXtransformations)
+
+    value = sc.scalar(2.4, unit='mm')
+    offset = sc.spatial.translation(value=[6, 2, 6], unit='mm')
+    vector = sc.vector(value=[0, 1, 1])
+    t = value.to(unit='m') * vector
+    write_translation(transformations, 't1', value, offset, vector)
+    expected1 = (sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit) *
+                 sc.spatial.translation(value=[0.006, 0.002, 0.006], unit='m'))
+
+    value = value * 0.1
+    t = value.to(unit='m') * vector
+    write_translation(transformations, 't2', value, offset, vector)
+    transformations['t2'].attrs['depends_on'] = 't1'
+    expected2 = (expected1 *
+                 sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit) *
+                 sc.spatial.translation(value=[0.006, 0.002, 0.006], unit='m'))
+
+    loaded = nxroot['transformations'][()]
+    assert set(loaded.keys()) == {'t1', 't2'}
+    assert sc.identical(loaded['t1'], expected1)
+    assert sc.allclose(loaded['t2'], expected2)
+
+
+def test_slice_transformations(nxroot):
+    transformations = nxroot.create_class('transformations', NXtransformations)
+    log = sc.DataArray(
+        sc.array(dims=['time'], values=[1.1, 2.2, 3.3], unit='m'),
+        coords={'time': sc.array(dims=['time'], values=[11, 22, 33], unit='s')})
+    log.coords['time'] = sc.epoch(unit='ns') + log.coords['time'].to(unit='ns')
+    offset = sc.spatial.translation(value=[1, 2, 3], unit='m')
+    vector = sc.vector(value=[0, 0, 1])
+    t = log * vector
+    t.data = sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit)
+    value1 = transformations.create_class('t1', NXlog)
+    value1['time'] = log.coords['time'] - sc.epoch(unit='ns')
+    value1['value'] = log.data
+    value1.attrs['transformation_type'] = 'translation'
+    value1.attrs['offset'] = offset.values
+    value1.attrs['offset_units'] = str(offset.unit)
+    value1.attrs['vector'] = vector.value
+
+    expected = t * offset
+
+    assert sc.identical(nxroot['transformations']['time', 1:3]['t1'], expected['time',
+                                                                               1:3])