BIFROST: Normalize by monitor and proton charge by jl-wynen · Pull Request #82 · scipp/essspectroscopy

jl-wynen · 2025-12-03T15:52:39Z

@g5t Please review even though this is a draft! I want to understand if this is the right approach.

This PR adds normalization by monitor and proton charge. We do not have a proton charge in our current files, so this is just set to 1pC.

The regression test is failing because I didn't update the reference data. I will do that once we agree that this is the right approach.

nvaytet · 2025-12-03T20:33:33Z

        NeXusMonitorName[FrameMonitor2]: '097_frame_2',
        NeXusMonitorName[FrameMonitor3]: '110_frame_3',
        PulsePeriod: 1.0 / sc.scalar(14.0, unit="Hz"),
+        ProtonCharge[SampleRun]: sc.DataArray(sc.scalar(1.0, unit='pC')),


Note that recent coda files have proton charge info under /entry/neutron_prod_info/pulse_charge.
See https://github.com/scipp/essimaging/blob/main/src/ess/tbl/orca.py#L31 for how it is loaded in essimaging.

That said, it appears to be 0 in the files, annoyingly...

Shouldn't we instead have the location of the nexus entry as a default param, and then in our tests/notebooks overwrite the ProtonCharge with wf[ProtonCharge[SampleRun]] = sc.DataArray(sc.scalar(1.0, unit='pC')).
This would make us more ready for when we actually have proton charge data in the files?

In addition, the proton charge should probably also be in the default_parameters (not just the simulation_default_parameters)?

g5t · 2025-12-04T12:45:31Z

+
+    .. math::
+
+        d_e^\\text{Norm} = d_e \\frac{\\Delta x_i}{m_i \\sum_j m_j} \\frac1{C}


I never realized braces are optional for (at least) \frac. You can omit the second pair too, \frac1C but I might argue in favour of \frac{1}{C} in case someone less familiar with LaTeX needs to edit this in the future.

Another small change, it would be great if the relationship between d_e and the i^th bin was more obvious by exchanging \lambda for x, indicating that \lambda_\text{i} is a property of the detected neutron, and (somehow) representing that there are many bins but only one corresponds to this wavelength

g5t · 2025-12-04T12:58:48Z

+        Monitor histogram.
+    proton_charge:
+        Accumulated proton charge.
+        Scalar or per setting (a3 / a4).


The proton charge needs to be per setting, which also includes the incident bandwidth selected by the choppers. The monitor histogram should also be dependent on the chopper settings.
So either this function can only work on one chopper-setting at a time, or the detector, monitor, and proton_charge all need a matching dimension for that.

""" detector: Detector events to normalize. Dimensions should include an axis for 'primary spectrometer setting' monitor: Monitor histogram. (wavelength, primary spectrometer setting) proton_charge: Accumulated proton charge. (secondary spectrometer setting, primary spectrometer setting) ... Note ----- Due to broadcasting, singular dimensions in, e.g, 'primary spectrometer setting' or 'secondary spectrometer setting' can be omitted. ... """

So far, all our workflows are meant to work with only one chopper setting. We expect that nexus files only contain one setting because otherwise, we would have to implement some expensive event filtering. Do you actually want to change settings in the middle of a run?

The proton charge needs to be per setting,

Yes. I am intending for it to get collected into an (a3, a4) array before it is passed into this function here. But I didn't implement it as we don't have proton charge logs yet.

Maybe the grand design has changed; but at some point years ago there was the idea that an entire experiment would end up in one NeXus file.
If this is no longer ECDC's vision, different primary-spectrometer settings could still end up in one file if a scan of those settings is the run (though, in such a case we'd likely be interested in the monitor data more than events in the detectors).

If the old idea persists, I hope that splitting the file into constant-primary-spectrometer-setting chunks should be straightforward; as in most cases one setting will be used for hours to days.

I don't think this is an ESS vision. As far as I remember ECDC said they'll more or less support what instrument scientists prefer, and as far as I can tell many (but not all) assume a start/stop mode.

g5t · 2025-12-04T13:04:17Z

+
+    norm = _monitor_distribution(detector=detector, monitor=monitor)
+    # Combine monitor and proton charge so we only operate on events once.
+    norm *= proton_charge


If norm is (number of wavelength bins) by (number of primary spectrometer settings) and proton_charge (number of primary spectrometer settings) by (number of secondary spectrometer settings) [with, maybe, 1000 by 2 and 2 by 360] will this broadcast still be efficient?
Or would it be better to keep the orthogonal parts separate and have, e.g., detector.bins / sc.lookup(norm) / sc.lookup(proton_charge) below?

I think it would still be efficient because we have many more events than elements in this array.

Why 1000 primary spectrometer settings? What does that setting entail for you?

My hypothetical numbers were 1000 wavelength bins, 2 primary spectrometer settings, 360 secondary spectrometer settings.
I'm happy that this isn't likely to be problematic.

And if it is, we can always change the code later.

This is needed when the proton charge is not a scalar because `norm` will have multiple dimensions.

nvaytet · 2025-12-10T13:20:48Z

        NeXusMonitorName[FrameMonitor2]: '097_frame_2',
        NeXusMonitorName[FrameMonitor3]: '110_frame_3',
        PulsePeriod: 1.0 / sc.scalar(14.0, unit="Hz"),
+        ProtonCharge[SampleRun]: sc.DataArray(sc.scalar(1.0, unit='pC')),


Shouldn't we instead have the location of the nexus entry as a default param, and then in our tests/notebooks overwrite the ProtonCharge with wf[ProtonCharge[SampleRun]] = sc.DataArray(sc.scalar(1.0, unit='pC')).
This would make us more ready for when we actually have proton charge data in the files?

In addition, the proton charge should probably also be in the default_parameters (not just the simulation_default_parameters)?

nvaytet · 2025-12-10T13:21:10Z

        NeXusMonitorName[FrameMonitor2]: '097_frame_2',
        NeXusMonitorName[FrameMonitor3]: '110_frame_3',
        PulsePeriod: 1.0 / sc.scalar(14.0, unit="Hz"),
+        UncertaintyBroadcastMode: UncertaintyBroadcastMode.fail,


Should UncertaintyBroadcastMode also be in simulation_default_parameters?

It is in there already.

jl-wynen · 2025-12-10T13:59:45Z

Shouldn't we instead have the location of the nexus entry as a default param, and then in our tests/notebooks overwrite the ProtonCharge with wf[ProtonCharge[SampleRun]] = sc.DataArray(sc.scalar(1.0, unit='pC')).
This would make us more ready for when we actually have proton charge data in the files?

In addition, the proton charge should probably also be in the default_parameters (not just the simulation_default_parameters)?

I didn't add the proton charge to the default params because of your first paragraph. But we don't have a provider to load it in ESSreduce yet and I didn't want to implement one here. So I left it out of the graph.

I added the proton charge directly in the simulation default params because we will never have an actual proton charge in those files. So I figured there is little reason to require users to set it every time.
Do you think it makes sense to set it in the notebooks just to show that it exists as a parameter for people that want to use coda / real data files?

nvaytet · 2025-12-11T13:08:50Z

Do you think it makes sense to set it in the notebooks just to show that it exists as a parameter for people that want to use coda / real data files?

I guess if it is not set, the workflow will complain; so people will find out about that parameter soon enough? 😉

jl-wynen requested a review from g5t December 3, 2025 15:52

jl-wynen added this to Development Board Dec 3, 2025

github-project-automation Bot moved this to In progress in Development Board Dec 3, 2025

jl-wynen moved this from In progress to Selected in Development Board Dec 3, 2025

nvaytet reviewed Dec 3, 2025

View reviewed changes

g5t reviewed Dec 4, 2025

View reviewed changes

jl-wynen added 5 commits December 8, 2025 15:38

Normalize by monitor and proton charge

71a6d82

Use explicit dim arg

1f5c401

This is needed when the proton charge is not a scalar because `norm` will have multiple dimensions.

Remove unused arg

ab9ff39

Update reference result

2d6374a

Remove TODO comment

a3a89ac

jl-wynen force-pushed the monitor-norm branch from 6d05f88 to a3a89ac Compare December 8, 2025 14:46

jl-wynen marked this pull request as ready for review December 8, 2025 14:46

jl-wynen requested a review from nvaytet December 9, 2025 07:45

nvaytet reviewed Dec 10, 2025

View reviewed changes

nvaytet approved these changes Dec 11, 2025

View reviewed changes

jl-wynen merged commit bb683e6 into main Dec 11, 2025
4 checks passed

jl-wynen deleted the monitor-norm branch December 11, 2025 13:22

github-project-automation Bot moved this from Selected to Done in Development Board Dec 11, 2025


		.. math::

		d_e^\\text{Norm} = d_e \\frac{\\Delta x_i}{m_i \\sum_j m_j} \\frac1{C}

Conversation

jl-wynen commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvaytet Dec 10, 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!

nvaytet Dec 10, 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!

jl-wynen commented Dec 10, 2025

Uh oh!

nvaytet commented Dec 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nvaytet Dec 10, 2025 •

edited

Loading

nvaytet Dec 10, 2025 •

edited

Loading