scipp / scippnexus

h5py-like utility for NeXus files with seamless scipp integration
https://scipp.github.io/scippnexus/
BSD 3-Clause "New" or "Revised" License
3 stars 3 forks source link

Improve loading of NXtransformations #100

Closed SimonHeybrock closed 1 year ago

SimonHeybrock commented 1 year ago

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.

jl-wynen commented 1 year ago

I'm not entirely clear on what is required here. Adapting an existing test, this seems to work:

def test_Transformation_with_single_value_no_depends_on(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
    expected = sc.spatial.translations(dims=t.dims, values=t.values, unit=t.unit)
    expected = expected * sc.spatial.translation(value=[0.006, 0.002, 0.006], unit='m')
    value = transformations.create_field('t1', value)
    value.attrs['transformation_type'] = 'translation'
    value.attrs['offset'] = offset.values
    value.attrs['offset_units'] = str(offset.unit)
    value.attrs['vector'] = vector.value

    t = nxtransformations.Transformation(nxroot['transformations/t1'])
    assert t.depends_on is None
    assert sc.identical(t.offset, offset)
    assert sc.identical(t.vector, vector)
    assert sc.identical(t[()], expected)
SimonHeybrock commented 1 year ago

Transformation is not what get's loaded directly (note it lacks the NX prefix and does not inherit NXobject. I think we probably want to add class NXtransformations(NXobject), and have it load its fields as Scipp spatial types. All the code for this exists in Transformation, but it needs to be pieced together, so it will be used when a (recursive) load encounters and NXtransformations group.

jl-wynen commented 1 year ago

Do you mean that nxroot['transformations/t1'][()] should return this transformation as a variable and nxroot['transformations'][()] should return a data group of transformations?

SimonHeybrock commented 1 year ago

Yes!