feat: pulse shaping #211

jokasimr · 2025-10-29T10:13:13Z

Fixes #205

…event_time_offset

…ameters

jl-wynen

Two overarching questions:

Is there a description of the maths somewhere? There are a lot of nontrivial equations here that don't seem to be explained.
How much of this is bespoke McStas code? We shouldn't mix that kind of code with code for measured data because that will make it difficult to know which functions to use once we have both.

jl-wynen · 2025-11-04T15:01:34Z

src/ess/beer/clustering.py

 from scippneutron.conversion.tof import dspacing_from_tof
 from scipy.signal import find_peaks, medfilt

+from .conversions import _t0_estimate, _time_of_arrival


Please don't import protected attributes. If you need them across modules, make them public. Possibly in a protected module if you don't want package users to use them.

src/ess/beer/conversions.py

jl-wynen · 2025-11-04T15:11:08Z

src/ess/beer/io.py

+    if 'lambda' in da.coords:
+        da.coords['wavelength_estimate'] = da.coords.pop('lambda')
+
+    if da.coords['wavelength_estimate'].value == '0':


This is bad because it scatters parameters and associations of modes with parameters over the code. Can you combine these parameters in one place? E.g., a mapping from mode to dict / dataclass of parameters? Then these long if-else chains can be collapsed into dict-lookups.

jokasimr · 2025-11-05T08:00:55Z

Is there a description of the maths somewhere? There are a lot of nontrivial equations here that don't seem to be explained.

You're right that it could use some more documentation.
But I think it's easier if you point to specific places where you find there is insufficient explanations.

The tof math for these pulse shaping chopper modes is:

Use a wavelength estimate to compute the time at the chopper.
Use the time at the chopper in combination with the time at the detector to compute the time of flight.

How much of this is bespoke McStas code? We shouldn't mix that kind of code with code for measured data because that will make it difficult to know which functions to use once we have both.

The McStas specific stuff should be contained in the io module. I don't think there is any McStas specific stuff outside of there, if there is it should be changed.

jl-wynen · 2025-11-05T09:06:19Z

The McStas specific stuff should be contained in the io module. I don't think there is any McStas specific stuff outside of there, if there is it should be changed.

True, but where will code for nexus files go? Also the io module?

jl-wynen · 2025-11-05T09:08:31Z

src/ess/beer/conversions.py

+    _eto = event_time_offset
+    T = sc.scalar(1 / 14, unit='s').to(unit=_eto.unit)
+    tc = tc.to(unit=_eto.unit)
+    return sc.where(_eto >= tc % T, _eto, _eto + T)


Why is there a modulo here? 'time cutoff' sounds like the condition should be _eto >= tc.

Yes you're right. tc should be in the interval [0, 1/14s], so the modulo does nothing.

Are there tests for this code? If not, can you add some? (I mean the additions in the PR overall.)

jl-wynen · 2025-11-05T09:08:49Z

src/ess/beer/conversions.py

+):
+    '''Does frame unwrapping based on the cutoff time `tc` and `event_time_ffset`.
+    Events before `tc` are assumed to come from the previous pulse.'''
+    _eto = event_time_offset


Suggested change

_eto = event_time_offset

eto = event_time_offset

It's unusual to use a protected local name like this.

I agree, but spellcheck complained about eto for some reason, so I renamed it.

src/ess/beer/conversions.py

jl-wynen · 2025-11-05T09:16:13Z

src/ess/beer/conversions.py

+    tc: sc.Variable,
+):
+    '''Does frame unwrapping based on the cutoff time `tc` and `event_time_ffset`.
+    Events before `tc` are assumed to come from the previous pulse.'''


Some general comments on docstring style:

According to https://peps.python.org/pep-0257/ they should always use double quotes.

Sphinx will use the entire first paragraph as the summary which is displayed in the overview tables:

These get unwieldy when the summary is long. So as a general rule, it is best to limit the summary to a single line and then use a separate paragraph for the detailed description. See also the [coding convention](https://scipp.github.io/development/coding-conventions.html#docstrings)

src/ess/beer/conversions.py

jl-wynen · 2025-11-05T09:23:43Z

src/ess/beer/io.py

    else:
-        raise ValueError('Sample position entry not found in file.')
-    data, events, params, sample_pos, chopper_pos = _load_h5(
+        raise ValueError(f'Unkonwn chopper mode {mode}.')


Which chopper's position is this? And why do we need to compute it? Isn't it written in the file?

It's the "effective" chopper position. It's either the position of one of the choppers, or the midpoint between two choppers.

But of which chopper? It looks like there is only a single position in the code even though there are many choppers. Is it the t0 chopper?

Good question.. After thinking a bit more about this, here is how I think it works:

The chopper system is designed so that, in pulse shaping mode,

there is one point along the beamline where every neutron has to pass within a short interval of time. That can be seen as a "virtual source" position or a "virtual chopper" position. This is "the chopper position" in the code.

a neutron emitted at the start of the pulse, with a particular wavelength, called lambda in the McStas files, reaches this virtual chopper exactly in the center of the time window when the chopper is open. That's why we can use the lambda parameter from the files to determine the opening time of the chopper.

I don't know if we are going to have access to this lambda parameter in practice, or if we are going to have to find the opening time of the virtual chopper by some other means.

I'm pretty sure we won't have lambda in the file. But isn't it a constant for a given chopper setting?

What about other modes? The code doesn't seem to only run for pulse shaping.

there is one point along the beamline where every neutron has to pass within a short interval of time. That can be seen as a "virtual source" position or a "virtual chopper" position. This is "the chopper position" in the code.

Can you change the name to reflect that and add a docstring?

What about other modes? The code doesn't seem to only run for pulse shaping.

For the multiplexing modes the neutron has to pass through one of a number of openings. But the concept of a virtual source with a position is still there.

I'm pretty sure we won't have lambda in the file. But isn't it a constant for a given chopper setting?

I'm not sure, I'll ask Celine about it.

Can you change the name to reflect that and add a docstring?

👍

Co-authored-by: Jan-Lukas Wynen <[email protected]>

jokasimr · 2025-11-05T09:54:04Z

True, but where will code for nexus files go? Also the io module?

Yes that's one option.

…into beer-pulse-shaping

jokasimr added the scipp-retreat-2025 label Oct 29, 2025

jokasimr added this to Development Board Nov 4, 2025

github-project-automation bot moved this to In progress in Development Board Nov 4, 2025

jokasimr moved this from In progress to Selected in Development Board Nov 4, 2025

jokasimr added 4 commits November 4, 2025 15:36

feat: pulse shaping

187f299

fix

ed8ba37

fix: load chopper settings in all modes - truncate time to limits of …

5aed46f

…event_time_offset

data: add updated simulation files to pooch, and add provider for par…

3cd12af

…ameters

jokasimr force-pushed the beer-pulse-shaping branch from 58cc140 to 3cd12af Compare November 4, 2025 14:41

jokasimr requested a review from jl-wynen November 4, 2025 14:41

docs

402382e

jl-wynen reviewed Nov 4, 2025

View reviewed changes

jokasimr added 3 commits November 5, 2025 09:11

fix: make functions public

7e7329d

docs

2ed9687

refactor: extract mode param mapping

a405435

jokasimr requested a review from jl-wynen November 5, 2025 08:37

jl-wynen reviewed Nov 5, 2025

View reviewed changes

Update src/ess/beer/conversions.py

ae279d4

Co-authored-by: Jan-Lukas Wynen <[email protected]>

jokasimr added 3 commits November 5, 2025 10:55

fix: unnecessary mod

96467a7

Merge branch 'beer-pulse-shaping' of github.com:scipp/essdiffraction …

a799d97

…into beer-pulse-shaping

docs

36544a5

jokasimr requested a review from jl-wynen November 5, 2025 12:32

jokasimr moved this from Selected to In progress in Development Board Nov 5, 2025

jokasimr added 2 commits November 6, 2025 09:31

refactor: change name to signal it makes a graph

d94cc37

docs: change quotes to double

f68032a

feat: pulse shaping #211

Are you sure you want to change the base?

feat: pulse shaping #211

Conversation

jokasimr commented Oct 29, 2025

Uh oh!

jl-wynen left a comment

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!

jokasimr commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jl-wynen commented Nov 5, 2025

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!

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!

jokasimr Nov 5, 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!

jokasimr commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jokasimr commented Nov 5, 2025 •

edited

Loading

jokasimr Nov 5, 2025 •

edited

Loading