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.

…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?

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 YooSunYoung and SimonHeybrock 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

Draft

		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

Are you sure you want to change the base?

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!

Uh oh!

nvaytet commented Jun 25, 2025 •

edited

Loading

nvaytet Jul 1, 2025 •

edited

Loading

nvaytet Jun 27, 2025 •

edited

Loading