Add customisation hook for loading time-dependent transformations from NeXus by jl-wynen · Pull Request #246 · scipp/ess

jl-wynen · 2026-03-16T08:45:55Z

This PR adds a hook to customise how time-dependent transformations are loaded from NeXus and merged with the data. This PR should not break any existing code. It only adds new options to the workflow.

See scipp/essspectroscopy#101 for a use case.

jl-wynen · 2026-03-18T09:37:11Z

This causes an issue with the Position domain type. It used to always be a (scalar) Variable. Now, when a component moves, its Position is a 1d data array. That means that the type should be

class Position(sciline.Scope[Component, RunType, sc.Variable | sc.DataArray], sc.Variable | sc.DataArray):

But that is not possible because we can't inherit from unions. So I'm thinking we could always make positions data arrays. But that impacts a lot of downstream code that currently expects variables. But that code is anyway broken for dime dependent positions and might need an update.

Alternatively, we could use a separate path through the workflow and add

class MovingPosition(sciline.Scope[Component, RunType, sc.DataArray], sc.DataArray):

and providers then have to request that type when they can deal with time dependence. The providers constructing Position would then raise if they get time-dependent inputs.

Thoughts? (@SimonHeybrock, @nvaytet)

Lookup relative error threshold

nvaytet · 2026-04-16T11:52:15Z

+def compute_dynamic_position(
+    transformation: NeXusTransformation[Component, RunType],
+) -> DynamicPosition[Component, RunType]:
+    """Compute the position of a component from a transformation matrix."""


Nitpick: Should there also be an error message if we attempt here to compute a dynamic position from a scalar variable?

I would say no. DynamicPosition can still be stationary. E.g., the BIFROST workflow should transparently handle moving and stationary detectors without changes to the graph.

nvaytet · 2026-04-16T11:58:13Z

+        params={
+            PreopenNeXusFile: PreopenNeXusFile(False),
+            TransformationTimeFilter: reject_time_dependent_transform,
+        },


Can you explain/sketch out what the e.g. Loki workflow would look like if we need to load both moving and stationary monitors?
I didn't see where the DynamicPosition is being used.

I didn't see where the DynamicPosition is being used.

It's unused here. But look at this PR: https://github.com/scipp/essspectroscopy/

As for Loki: You don't need to do anything for stationary monitors. Let's say, Monitor2 moves. Then you need to set

def monitor_time_filter(transform: sc.DataArray) -> sc.DataArray: ... def get_calibrated_dmoving_monitor( monitor: NeXusComponent[Monitor2, RunType], *, transform: NeXusTransformation[Monitor2, RunType], ) -> EmptyMonitor2[Monitor2, RunType]: ... wf[TransformationTimeFilter[Monitor2, RunType]] = monitor_time_filter wf.insert(get_calibrated_dmoving_monitor)

This overrides the default for Monitor2 and allows you to specify how to handle the time-dependence.

Why do we need to do 2 things to the workflow? (1. set TransformationTimeFilter, 2. insert a new provider)
Can the filtering just be done inside the provider?

Or maybe it's because inserting the provider might be something added in e.g. essspectroscopy inside the module, but the time filter may be tweaked by the user in the notebook, hence the separation?

But look at this PR: https://github.com/scipp/essspectroscopy/

Can you provide a more specific link? 😉

This is the filter: https://github.com/scipp/essspectroscopy/blob/6bee451edceef4c9b0097180d064a79f2dec53f9/src/ess/bifrost/io/nexus.py#L178
This is how the data is combined: https://github.com/scipp/essspectroscopy/blob/6bee451edceef4c9b0097180d064a79f2dec53f9/src/ess/bifrost/detector.py#L83

nvaytet · 2026-04-16T11:59:58Z

-        'source_position': lambda: source_position,
-        'sample_position': lambda: sample_position,
+        'source_position': lambda: source_position.position,
+        'sample_position': lambda: sample_position.position,


I'm wondering if this can potentially break a lot of existing code?

Yes. That should not have been there anymore. I guess I forgot to push something. See the update.

For context: This PR should not break any existing code. It loads 'static' positions as before using the Position type. It adds the DynamicPosition type that allows for potentially time-dependent data. But you need to explicitly request that in the workflow. (Used for BIFROST analysers)

SimonHeybrock · 2026-04-17T07:14:32Z

                sizes=t.sizes, coord=time, index=interval.value
            )
-            t.value = _time_filter(t.value[idx])
+            t.value = _time_filter(t.value[idx], time_filter)


Confusing near-duplicate variable naming. Should _time_filter by _apply_time_filter or something like that?

SimonHeybrock · 2026-04-17T07:19:53Z



+def compute_dynamic_position(
+    transformation: NeXusTransformation[Component, RunType],


The to_transformation docstring returning this still claims it is time-independent?

Which line are you referring to? to_transformation can indeed return a time-dependent transform. But that depends on TransformationTimeFilter.

SimonHeybrock · 2026-04-17T07:28:13Z

@jl-wynen regarding #246 (comment)

Ah, right. Fixed it

nvaytet · 2026-04-17T07:28:34Z

    assert sc.identical(transform * origin, sc.vector([1.0, 3.0, 0.0], unit='m'))


-def test_to_transform_raises_if_interval_does_not_yield_unique_value(


Do I remember correctly that there used to be some tests that you added in this PR, but now it seems they are gone?
Or am I somehow reading the wrong diff?

They are gone. I could add them back but that requires implementing the custom function I outlined in the Loki comment which makes the tests somewhat complicated. Should I do that? Or is it enough to have the bifrost tests which will soon be in the monorepo?

I guess if one day someone decides to remove the tests in Bifrost, we may forget that reduce was relying on those, and we would then no longer be testing this anywhere?

I guess it depends how complicated the tests get...

True. I restored a test I accidentally removed and added one more. It only tests to_transformation. Do we need a full workflow test? That would mean making the input data.

Use a list as arg to drop_coords

SimonHeybrock · 2026-04-17T09:56:28Z

@jl-wynen Before merging, please also update the PR description!

The previous attempt added the time as an event coord. This makes grouping by time-dependent coords more difficult down the line. It is not apparent which event coords depend on a slow timescale that we might later want to group or bin into and which coords truly depend on the events.

Needed for vector lookup.

jl-wynen mentioned this pull request Mar 16, 2026

[WIP] transformation resolution for time-dependent BIFROST scipp/essreduce#321

Closed

jl-wynen force-pushed the nxs-time-dependent-transform branch from 021f15c to fbd43c4 Compare March 17, 2026 10:29

jl-wynen force-pushed the nxs-time-dependent-transform branch from 2a49cd8 to 407d815 Compare April 10, 2026 09:14

jl-wynen mentioned this pull request Apr 10, 2026

[BIFROST] Implement support for a moving detector scipp/essspectroscopy#101

Merged

nvaytet pushed a commit that referenced this pull request Apr 15, 2026

Merge pull request #246 from scipp/lookup-error-threshold

c236063

Lookup relative error threshold

jl-wynen force-pushed the nxs-time-dependent-transform branch from 251fd99 to 7a0234c Compare April 16, 2026 09:58

jl-wynen marked this pull request as ready for review April 16, 2026 10:54

jl-wynen force-pushed the nxs-time-dependent-transform branch from 8720cee to 153a588 Compare April 16, 2026 11:05

nvaytet reviewed Apr 16, 2026

View reviewed changes

jl-wynen force-pushed the nxs-time-dependent-transform branch from b5cae84 to f6ff090 Compare April 17, 2026 07:10

SimonHeybrock reviewed Apr 17, 2026

View reviewed changes

Comment thread packages/essreduce/src/ess/reduce/nexus/workflow.py

SimonHeybrock reviewed Apr 17, 2026

View reviewed changes

nvaytet reviewed Apr 17, 2026

View reviewed changes

jokasimr pushed a commit that referenced this pull request Apr 17, 2026

Merge pull request #246 from scipp/fix-use-list-arg

7e8bdde

Use a list as arg to drop_coords

SimonHeybrock approved these changes Apr 17, 2026

View reviewed changes

jl-wynen force-pushed the nxs-time-dependent-transform branch from 0d47a53 to 782c18e Compare April 17, 2026 11:08

nvaytet approved these changes Apr 17, 2026

View reviewed changes

jl-wynen changed the title ~~Load time-dependent transformations from NeXus~~ Add customisation hook for loading time-dependent transformations from NeXus Apr 20, 2026

jl-wynen added 8 commits April 20, 2026 09:14

Load time-dependent transformations

e34c4ae

Update NXtransformation docs

2f58b7c

Use tighter lower bound

930c1d2

Bump scipp to 26.3.1

9f50700

Needed for vector lookup.

Fix test for time dependent transform

a820414

Remove nx_class_for_crystal

38e7f50

Add dedicated Position class

eda6882

jl-wynen added 8 commits April 20, 2026 09:15

Make time_filter a public parameter

5786260

Do not broadcast in time by default

846c411

Split Position and DynamicPosition

c983c9c

Fixup: Remove .position access

73341dc

Use clearer name

96b6b26

Remove unused import

3c59b32

Update docstring

a4c0980

Restore time-dependent tests

74b33c0

jl-wynen force-pushed the nxs-time-dependent-transform branch from 782c18e to 74b33c0 Compare April 20, 2026 07:15

jl-wynen enabled auto-merge April 20, 2026 07:16

jl-wynen merged commit 594ed3f into main Apr 20, 2026
13 checks passed

jl-wynen deleted the nxs-time-dependent-transform branch April 20, 2026 07:33



		def compute_dynamic_position(
		transformation: NeXusTransformation[Component, RunType],

		assert sc.identical(transform * origin, sc.vector([1.0, 3.0, 0.0], unit='m'))


		def test_to_transform_raises_if_interval_does_not_yield_unique_value(

Conversation

jl-wynen commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jl-wynen commented Mar 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jl-wynen Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonHeybrock commented Apr 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jl-wynen commented Mar 16, 2026 •

edited

Loading

jl-wynen Apr 17, 2026 •

edited

Loading