[ESSDIFFRACTION] feat: add workflow using lookup tables for pulse shaping modes#647
Conversation
github-actions
Bot
changed the title
… so t=0 corresponds to pulse center)
| List of ESS BEER choppers for the selected pulse-shaping mode. | ||
|
|
||
| We make the chopper information available in this way as loading it directly from | ||
| the NeXus files is currently not available for these simulated BEER data. |
There was a problem hiding this comment.
Should the function have a name that clarifies this to avoid people accidentally using it over chopper info from files? Something like simulation_choppers or default_choppers?
| unit=wavelength_estimate.unit | ||
| ) | ||
| ).to(unit='s') | ||
| ).to(unit='s') + sc.scalar(2.86, unit='ms').to(unit='s') / 2 |
There was a problem hiding this comment.
What is this number? And why did you add it now?
There was a problem hiding this comment.
Because I noticed that in the beer McStas model the t event data value (time of detection) is modified by a offset value that is half a ess pulse width. This makes the source pulse centered around t=0 in the McStas model. To convert that t value to event_time_offset with the convention that we expect (event_time_offset=0 corresponds to the start of a pulse) I now add the same offset to event_time_offset when loading McStas data. And to keep the results unchanged we have to shift the time here by the same offset.
I think this makes more sense than the old version. The t0_estimate is supposed to be the time a "nominal" wavelength neutron reaches the wavelength defining chopper. But that time is not defined unless we also specify a "nominal" time the neutron was created at the source. Before this change that time was 0, now it is source_pulse_width / 2, this is more in line with the conventions I'm used to.
There was a problem hiding this comment.
Ok. Can you add this explanation to the code? It's hard to find in a GH comment.
| ) | ||
| def __getattr__(name: str): | ||
| """Return attributes from the replacement :mod:`ess.beer.mcstas` module.""" | ||
| return getattr(_mcstas, name) |
There was a problem hiding this comment.
Can you raise a deprecation warning?
There was a problem hiding this comment.
Did anything change in this file? The diff isn't usable.
There was a problem hiding this comment.
Two things changed:
- In the
load_beer_mcstasfunction the definition ofevent_time_offsetchanged. Now a small offset ofpulse_width/2is added, so that we have the convention that the source pulse starts atevent_time_offset=0. Theframe_cutoff_timewas also shifted correspondingly. - New providers needed for the lookup table frame unwrapping workflows are added after line 530 in the file.
| # But that is different from the convention that we have for | ||
| # `event_time_offset`. We expect `event_time_offset=0` to correspond | ||
| # to the start of a pulse. | ||
| # To fix this, half a pulse width is added to `t`. |
There was a problem hiding this comment.
Please also add a short comment (maybe with a link to this one) in the other place where you add the offset.
There was a problem hiding this comment.
Nah I don't think that is necessary because the logic there is completely natural when we work with the standard convention that the start of the pulse is event_time_offset=0.
There was a problem hiding this comment.
But it uses a 'magic' number. I'd like to at least have an explanation what that number represents.
Co-authored-by: Jan-Lukas Wynen <jan-lukas.wynen@ess.eu>
jokasimr
force-pushed
the
add-pulse-shaping-choppers
branch
from
7d5d7da to
38bbbfc
Compare
… parameters to account for shift in t (convention change)
2 participants
Fixes #628
Chopper information taken from https://git.esss.dk/dram/code-shelf/code-shelf/-/blob/main/src/beer/notebooks/commissioning/beer_choppers_tof.py?ref_type=heads