[ESSDIFFRACTION] BEER improvements - #704
Conversation
github-actions
Bot
changed the title
|
The changes look large and scary because of the lockfile, the non-lockfile changes are something like +850 -950. |
|
Here is the 
|
|
Comments from in-person review:
|
There was a problem hiding this comment.
What is the purpose of these tests? It seems to me like you are just checking that you managed to type in the same hard-coded numbers in the tests and the module. The tests do not check any behaviour. And whenever parameters change, the tests need to change as well.
There was a problem hiding this comment.
I agree they're not useful, let's remove them.
They were originally ai-written, and I thought about removing them then, but in the end I didn't do it.
| } | ||
|
|
||
|
|
||
| def _beer_modulation_workflow(graph_provider, *providers) -> sl.Pipeline: |
There was a problem hiding this comment.
Can you rename this to reflect that it is only valid for McStas data?
| t0 += 0.5 | ||
| sc.floor(t0, out=t0) | ||
| t0 *= mod_period | ||
| t -= t0 |
There was a problem hiding this comment.
Please add a comment explaining t0 (in relation to subframes and tof, ltotal)
| from ..types import DetectorBank, Filename, RawDetector | ||
| from .beamline import ModulationMode, PulseShapingMode, simulation_choppers | ||
|
|
||
| _MCSTAS_T_OFFSET = sc.scalar(1.6, unit='ms') |
There was a problem hiding this comment.
Please add a comment explaining what this is.
There was a problem hiding this comment.
It's a specific offset applied in the beer mcstas model that changes the mcstas convention of what t=0 corresponds to. (The convention is that it corresponds to the beginning of the pulse, but in beer mcstas files it corresponds to the center of the pulse.)
There was a problem hiding this comment.
I'll add a comment.
|
@jl-wynen was there anything more to do here? |
2 participants
Main changes are: