Create OdinWorkflow and use new NeXus files #91

nvaytet · 2025-06-25T20:18:35Z

Now that we have new NeXus files created by the mcstas-to-nexus notebook from #90, we can create a OdinWorkflow based on the generic Tof workflow from essreduce.

We replace most of the Odin notebook in the docs with a shorter and more-production-like notebook.

I also moved the TBL types to a common imaging.types so odin could use them as well.

TODO: we should use normalisation functions from the imaging/normalize submodule to do the normalization at the end. But we defer this to another PR.

Needs scipp/essreduce#253

…notebook

…iles came from

YooSunYoung

New names are more clear...!

SimonHeybrock · 2025-06-27T04:29:34Z

src/ess/imaging/types.py

+class DetectorWavelengthData(sciline.Scope[RunType, sc.DataArray], sc.DataArray):
+    """Detector data with wavelength coordinate."""


We should really try to come up with canonical naming across projects. For example, this is call CountsWavelength in essdiffraction and something like CleanWavelength in esssans.

Yes, which one do we think is the best? I find CleanWavelength confusing personally.

Maybe we should just be much more explicit: DetectorDataWithWavelengthCoordinate or something like that?

I think the DetectorWavelengthData came from the fact that we have named DetectorTofData the data that comes out of the tof lookup in essreduce, so it seemed natural.
But I'm happy to go with Counts or anything we find better?

Yes, which one do we think is the best? I find CleanWavelength confusing personally.

Agree, it should be replaced.

Maybe we should just be much more explicit: DetectorDataWithWavelengthCoordinate or something like that?

I don't think that is going to work. (a) it is unusable and horrible UX, (b) you are never going to encode the full "contract" in the name. For me, CountsWavelength is kind of a short form of DetectorDataWithWavelengthCoordinate. No one cares it is a coordinate. It might not even be a real "coord" if we are talking about events.

it is unusable and horrible UX

Sounds a bit harsh, but whatever...

No one cares it is a coordinate. It might not even be a real "coord" if we are talking about events.

That is true.

For me, CountsWavelength is kind of a short form of DetectorDataWithWavelengthCoordinate.

I really don't mind what we use, it was just in case we were not entirely happy with CountsWavelength. I'd just like to move forwards with the PR, so I'll just go with CountsWavelength.

SimonHeybrock · 2025-06-27T04:31:34Z

src/ess/odin/beamline.py

+
+
+def choppers(source_position: sc.Variable) -> MappingProxyType[str, DiskChopper]:
+    """Hard-coded DISK_CHOPPERS dictionary for ESS ODIN choppers."""


I can see that. Maybe an explanation of why that is? Will this be the only setting?

They have not yet communicated how many settings they will have.

SimonHeybrock · 2025-06-27T04:33:15Z

src/ess/odin/masking.py

+        if (out.bins is not None) and (coord_name in out.bins.coords):
+            out.bins.masks[coord_name] = mask(out.bins.coords[coord_name])
+        else:
+            out.masks[coord_name] = mask(out.coords[coord_name])


Won't/couldn't this lead to bin-edge masks?

True. I copied it from Essdiffraction.
Is it enough to check if the coord is bin edges and then apply the function on the midpoints?

Does this mean you currently have masks on edges in the live workflow when you added a tof mask?

SimonHeybrock · 2025-06-27T04:34:42Z

src/ess/imaging/types.py

+DarkBackgroundRun = reduce_t.BackgroundRun
+DetectorData = reduce_t.DetectorData
+DiskChoppers = reduce_t.DiskChoppers
+OpenBeamRun = reduce_t.EmptyBeamRun
+Filename = reduce_t.Filename
+FrameMonitor1 = reduce_t.FrameMonitor1
+FrameMonitor2 = reduce_t.FrameMonitor2
+FrameMonitor3 = reduce_t.FrameMonitor3
+MonitorData = reduce_t.MonitorData
+NeXusDetectorName = reduce_t.NeXusDetectorName
+NeXusMonitorName = reduce_t.NeXusName
+NeXusComponent = reduce_t.NeXusComponent
+SampleRun = reduce_t.SampleRun


I wonder if we want to keep using these aliases. I remember it can lead to some confusion in visualizations and error messages?

Imaging people use the terminology "dark" run and "open beam" run.
It would be nice if we can use these names in the workflow...

I'm happy to do away with all the other aliases.

If we remove the aliases, does this mean we end up with something like this in the notebooks:

from ess.reduce.nexus import types as nxs_t from ess.reduce.time_of_flight import types as tof_t from ess.imaging import types as img_t wf[nxs_t.Filename[img_t.OpenBeamRun]] = odin.data.iron_simulation_ob_small() wf[nxs_t.NeXusDetectorName] = "event_mode_detectors/timepix3" raw_data = wf.compute(nxs_t.DetectorData[img_t.OpenBeamRun]) tof_data = wf.compute(nxs_t.DetectorTofData[img_t.OpenBeamRun])

It's not so great that we have to remember which submodule contains which type.

Can you elaborate on what were the confusions in the visualizations and error messages?

Have you tried workflow.visualize() when there are aliases in place?

So as long as the alias has the same name as the underlying type, I think it looks fine?

But when I tried with the OpenBeamRun which was using the EmptyBeamRun from essreduce under the hood, it displays EmptyBeamRun instead:

But I guess I should fix this by making new types here, instead of using the ones from essreduce, as you suggest in your comment further down?

So as long as the alias has the same name as the underlying type, I think it looks fine?

It looks fine, but as soon as we decide to display the full module name it won't?

SimonHeybrock · 2025-06-27T04:36:27Z

src/ess/imaging/types.py

+DetectorLtotal = tof_t.DetectorLtotal
+DetectorTofData = tof_t.DetectorTofData
+PulsePeriod = tof_t.PulsePeriod
+PulseStride = tof_t.PulseStride
+PulseStrideOffset = tof_t.PulseStrideOffset
+DistanceResolution = tof_t.DistanceResolution
+TimeResolution = tof_t.TimeResolution
+LtotalRange = tof_t.LtotalRange
+LookupTableRelativeErrorThreshold = tof_t.LookupTableRelativeErrorThreshold
+NumberOfSimulatedNeutrons = tof_t.NumberOfSimulatedNeutrons
+TimeOfFlightLookupTable = tof_t.TimeOfFlightLookupTable
+SimulationResults = tof_t.SimulationResults


Isn't it confusing that the tof parameters are now at the same level as the others? Given the names it feels odd to have, say, ess.imaging.type.TimeResolution. Do we really need the aliases? If so, should it be prefixed, or in a submodule?

But see also my concern above.

This is again copied from essdiffraction...

That is not really an argument for doing it?

No, apart from the fact that it had been reviewed and merged and therefore I thought it was our currently accepted best practice 😉

SimonHeybrock · 2025-06-27T04:39:17Z

src/ess/imaging/types.py

+DarkBackgroundRun = reduce_t.BackgroundRun
+DetectorData = reduce_t.DetectorData
+DiskChoppers = reduce_t.DiskChoppers
+OpenBeamRun = reduce_t.EmptyBeamRun
+Filename = reduce_t.Filename
+FrameMonitor1 = reduce_t.FrameMonitor1
+FrameMonitor2 = reduce_t.FrameMonitor2
+FrameMonitor3 = reduce_t.FrameMonitor3
+MonitorData = reduce_t.MonitorData


After improvements in Sciline there is no more need to use run or monitor types from ess.reduce. The NeXus workflow will work with any defined here. Unless we are absolutely sure that the renaming is fully opaque renaming, e.g., run types might cause a lot of confusion.

I'm running into an issue though. I removed importing RunType, SampleRun, EmptuBeamRun etc from essreduce.
Instead, defined

SampleRun = NewType("SampleRun", int) DarkBackgroundRun = NewType("DarkBackgroundRun", int) OpenBeamRun = NewType("OpenBeamRun", int) RunType = TypeVar("RunType", SampleRun, DarkBackgroundRun, OpenBeamRun)

Now, when I try to compute something using the OdinWorkflow which is based on the GenericTofWorkflow, I get things like

UnsatisfiedRequirement: Missing input node 'DiskChoppers[SampleRun]'. Affects requested targets (via providers given in parentheses): 1. DiskChoppers[SampleRun] → (ess.reduce.time_of_flight.simulation.simulate_chopper_cascade_using_tof) → SimulationResults → (ess.reduce.time_of_flight.eto_to_tof.compute_tof_lookup_table) → TimeOfFlightLookupTable

because in the GenericTofWorkflow, it is expecting the SampleRun from essreduce, but I only set the one from essimaging on the workflow, so it can't find SampleRun...

Am I doing something wrong?

@jl-wynen Any idea what is going on? I think you implemented and used this more.

Can you point me at an example. I don't see DiskCoppers in the graph.

This may be unrelated to the type vars. We used to (incorrectly) load DiskChopper objects directly from NeXus. But we have to process the loaded data first, e.g., extract scalars from time series logs. See scipp/essreduce#242

The workflow works fine if I use the SampleRun from essreduce instead of using a new SampleRun that I created inside ess.odin.types.

Can you post an example of how you use the workflow? And is it the latest version on this branch?

from ess.reduce import time_of_flight as tof from ess.reduce.nexus import types as red_t from typing import NewType, TypeVar import scipp as sc default_parameters = { tof.PulsePeriod: 1.0 / sc.scalar(14.0, unit="Hz"), tof.PulseStride: 2, tof.PulseStrideOffset: None, tof.LookupTableRelativeErrorThreshold: 0.1, tof.LtotalRange: (sc.scalar(55.0, unit="m"), sc.scalar(65.0, unit="m")), tof.DistanceResolution: sc.scalar(0.1, unit="m"), tof.TimeResolution: sc.scalar(250.0, unit='us'), } SampleRun = NewType("SampleRun", int) DarkBackgroundRun = NewType("DarkBackgroundRun", int) OpenBeamRun = NewType("OpenBeamRun", int) RunType = TypeVar("RunType", SampleRun, DarkBackgroundRun, OpenBeamRun) wf = tof.GenericTofWorkflow( run_types=[SampleRun, OpenBeamRun], monitor_types=[], tof_lut_provider=tof.TofLutProvider.TOF ) for key, param in default_parameters.items(): wf[key] = param wf[red_t.DiskChoppers[SampleRun]] = {} table = wf.compute(tof.TimeOfFlightLookupTable)

yields

UnsatisfiedRequirement: Missing input node 'DiskChoppers[SampleRun]'. Affects requested targets (via providers given in parentheses): 1. DiskChoppers[SampleRun] → (ess.reduce.time_of_flight.simulation.simulate_chopper_cascade_using_tof) → SimulationResults → (ess.reduce.time_of_flight.eto_to_tof.compute_tof_lookup_table) → TimeOfFlightLookupTable

Ok, this makes sense. You define a custom SampleRun. This is a different class from what is defined in ESSreduce. But simulate_chopper_cascade_using_tof requires the type from ESSreduce. So your custom one which you use to insert DiskChoppers[SampleRun] does not match.

In general, if ESSreduce, or any upstream code, requires a specific run type, you have to use that exact class. We only have the flexibility to define our own downstream if upstream does not directly depend on them. E.g., if simulate_chopper_cascade_using_tof requested DiskChoppers[RunType], you would be fine.

All that to say, use the run types provided by ESSreduce and add your own to them when you constrain RunType. See how this is done in ESSdiffraction: https://github.com/scipp/essdiffraction/blob/9cbe15489cb16954de7388f712a9a55d32833e00/src/ess/powder/types.py#L52-L60

Does this mean that we should not have any SampleRun etc anywhere in essreduce? Only RunType?

Basically if I am unable to use SampleRun from essreduce (for a naming reason), then I cannot use the generic tof workflow?

I guess using the choppers from the SampleRun in essreduce was just a hack, because we (I) were too lazy to either implement something that would check that chopper settings were the same for all runs, or have one table per run.

Thinking about it now, there is potentially no reason why one could not have different chopper settings for different runs in the same workflow? As long as you have converted to physical coordinates such as wavelength before you e.g. normalize, then you could have one LUT for SampleRun and another for EmptyBeamRun.

So maybe we just want to have RunType everywhere?

YooSunYoung · 2025-07-11T08:53:33Z

I'm kind of lost...
It's not so easy to follow what we are trying to fix from the PR or how we are fixing the problem...
Can we have a brief session about the scope of GenericWorkflow, NexusWorkflow and right way of using RunTypes and so on...?

(And I'm sorry @nvaytet that I should have thought about it earlier
but we probably shouldn't just delete the existing one that directly uses McStas simulation data
as it's a waste to convert them to NeXus every time people want to reduce the simulation data.
But we can just add them back again later...)

nvaytet · 2025-07-11T12:45:06Z

I'm kind of lost...
It's not so easy to follow what we are trying to fix from the PR or how we are fixing the problem...
Can we have a brief session about the scope of GenericWorkflow, NexusWorkflow and right way of using RunTypes and so on...?

Sure, we can have such a session.
In short, to answer what we are trying to do here is use the GenericTofWorkflow for Odin.
I made a LUT so we can just load it from file, and not have to bother with creating it every time we run a reduction.
I also added the notebook that was used to create the LUT (as we have in essdiffraction), which is based on the new TofLookupTableWorkflow in essreduce.

as it's a waste to convert them to NeXus every time people want to reduce the simulation data.

Why? I think it's simple enough to use the mcstas-to-nexus notebook to convert one or multiple mcstas simulations to nexus files? You then just use the same workflow for real or simulated data, you don't have to swap providers in and out?

YooSunYoung · 2025-07-11T12:54:07Z

Sure, we can have such a session.

👍

In short, to answer what we are trying to do here is use the GenericTofWorkflow for Odin.

That much I understood but I didn't quite follow why all the type names also had to be updated and etc... maybe I'll just go through it slowly again...
Thank you for the kind answer 🕺

as it's a waste to convert them to NeXus every time people want to reduce the simulation data.

Why? I think it's simple enough to use the mcstas-to-nexus notebook to convert one or multiple mcstas simulations to nexus files? You then just use the same workflow for real or simulated data, you don't have to swap providers in and out?

We can just have a pre-made workflow so that we don't have to swap providers in and out.
I think mcstas-to-nexus should be only for validating the real workflow not the standard way of reducing mcstas simulation data but... maybe it doesn't matter 🤷
But as I said, we can discuss it later. It's not important to this PR.

nvaytet · 2025-07-11T13:16:35Z

We can just have a pre-made workflow so that we don't have to swap providers in and out.

Yes, sorry that's not really what I meant to say. What I meant was that we don't have to maintain a parallel set of providers (= a second workflow), which is basically all the code in odin_mcstas_helper.py. That just goes away, which is quite nice?

I didn't quite follow why all the type names also had to be updated

Hmm I don't think I updated any types names when we compare these changes to the main branch?
Some types in odin_mcstas_helper.py are gone, but that's because they are no longer used.

Compared to the first version of the PR, I am importing many less types from essreduce because all the ones that were dealing with creating a LUT on-the-fly from the chopper parameters are now gone (actually they are now living in the tools/odin-make-tof-lookup-table.ipynb notebook).

Then, instead of using the SampleRun from essreduce, and the essreduce.EmptyBeamRun under the alias OpenBeamRun (which is the terminology imaging people prefer to use) etc. I just created the run types here, and made the typevar RunType from them, which works since the recent changes in sciline.
The reason to not use the essreduce.EmptyBeamRun under the OpenBeamRun alias is that if you do that, and then do workflow.visualize(), the names that appear on the diagram are EmptyBeamRun instead of OpenBeamRun, which is very confusing for the users.

YooSunYoung · 2025-07-14T13:08:40Z

We can just have a pre-made workflow so that we don't have to swap providers in and out.

Yes, sorry that's not really what I meant to say. What I meant was that we don't have to maintain a parallel set of providers (= a second workflow), which is basically all the code in odin_mcstas_helper.py. That just goes away, which is quite nice?

I wasn't talking about removing the helper module. I was talking about the notebook itself for simulation data reduction.
But as I said, we can add it back later so not so important here.

Compared to the first version of the PR, I am importing many less types from essreduce because all the ones that were dealing with creating a LUT on-the-fly from the chopper parameters are now gone (actually they are now living in the tools/odin-make-tof-lookup-table.ipynb notebook).

Ah now I understood. Also about the RunType.
I wasn't sure what to do with those RunType since Søren and Robin said ODIN would record all different RunTypes in a single file.... But I guess in that case we can prune the GenericWorkflow... and add a provider that retrieve each RunType...? (Also sth just popped into my mind, not sth I want to discuss here)

…up-to-date and update docstring in data registry to record how many neutrons were used to create the tables

nvaytet · 2025-08-05T12:09:49Z

@SimonHeybrock could you take another look after the update to just use LUT from file? 🙏

SimonHeybrock · 2025-08-05T12:47:04Z

No more comments (looking only at the non-docs/non-test code).

nvaytet and others added 6 commits June 25, 2025 13:11

make odin submodule and create Odin workflow

7ce4671

introduce masking in the workflow and replace notebook with new odin …

faac039

…notebook

Apply automatic formatting

5e9f279

static analysis

2cb0f29

fix tbl tests

37796d2

add tests for odin workflow

298646a

nvaytet marked this pull request as ready for review June 25, 2025 20:45

nvaytet requested review from SimonHeybrock and YooSunYoung June 26, 2025 08:42

nvaytet and others added 5 commits June 26, 2025 11:11

begin renaming EmptyBeam to OpenBeam

dbcae2c

finish renaming EmptyBeamRun

3ace30c

fix TBL notebook and add more details in docstrings as to where the f…

fcb2427

…iles came from

Apply automatic formatting

5fb5256

Merge branch 'main' into use-new-nexus-files

33644f9

YooSunYoung approved these changes Jun 26, 2025

View reviewed changes

SimonHeybrock reviewed Jun 27, 2025

View reviewed changes

nvaytet added 3 commits June 27, 2025 12:02

make OdinGenericWorkflow and a bragg edge workflow

11b11fd

better docstring for choppers

b21c7f8

handle masks on edges

b7e5ced

nvaytet mentioned this pull request Jul 4, 2025

Use RunType instead of SampleRun for tof lookup table scipp/essreduce#251

Closed

nvaytet and others added 8 commits July 9, 2025 17:05

move workflows out of subfolder into a single file

dd9f655

update odin reduction notebook

956e592

Apply automatic formatting

c8607ed

fix tests

e6daa77

cleanup

6975f19

fix static analysis

9c8ba81

fix import

14e8ed4

use correct name for workflow

445f62e

nvaytet added 9 commits July 15, 2025 11:50

bump essreduce version

4225f47

update deps

6f5ca5c

make tbl tests use lut from file

3007c90

add notebook to create TBL lut

2ab7f23

update TBL docs notebook

3bb7154

bump essreduce version in the coorect place

1e4ee71

update deps

8f05bb1

move notebooks for generating tof LUTs to docs so that they are kept …

851ea0e

…up-to-date and update docstring in data registry to record how many neutrons were used to create the tables

remove readme fetch and add retries for odin and tbl pooches

cf22d9c

nvaytet merged commit 74d4b11 into main Aug 5, 2025
4 checks passed

nvaytet deleted the use-new-nexus-files branch August 5, 2025 12:51

		class DetectorWavelengthData(sciline.Scope[RunType, sc.DataArray], sc.DataArray):
		"""Detector data with wavelength coordinate."""



		def choppers(source_position: sc.Variable) -> MappingProxyType[str, DiskChopper]:
		"""Hard-coded DISK_CHOPPERS dictionary for ESS ODIN choppers."""

Create OdinWorkflow and use new NeXus files #91

Create OdinWorkflow and use new NeXus files #91

Uh oh!

Conversation

nvaytet commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YooSunYoung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvaytet Jul 1, 2025 • 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!

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!

nvaytet Jun 27, 2025 • 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!

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!

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!

YooSunYoung commented Jul 11, 2025

Uh oh!

nvaytet commented Jun 25, 2025 •

edited

Loading

nvaytet Jul 1, 2025 •

edited

Loading

nvaytet Jun 27, 2025 •

edited

Loading