Conversation
…ank index with enum
| return { | ||
| **gg, | ||
| 't0': t0_estimate, | ||
| 'tof': lambda time_of_arrival, t0: time_of_arrival - t0, |
There was a problem hiding this comment.
Eventually we will replace this mechanism for computing wavelengths in beer pulse shaping modes with the lookup table approach to make it consistent with the other techniques.
| if north_or_south is not None and number is not None | ||
| else next(_find_all_h5(data_dir, f'.*{north_or_south}')) | ||
| if north_or_south is not None | ||
| else next(_find_all_h5(data_dir, f'/entry1.*bank.*{number}')) |
There was a problem hiding this comment.
The McStas file loading is complex because there have been several rounds of changes to the McStas output files, and we want to be "backwards compatible" with the old McStas output structure because otherwise we have to replace the files in the tests/docs.
I've raised this with Celine and I don't expect there will be more significant changes going forward.
| def t0_estimate( | ||
| wavelength_estimate: sc.Variable, | ||
| L0: sc.Variable, | ||
| Ltotal: sc.Variable, | ||
| ) -> sc.Variable: | ||
| """Estimates the time-at-chopper by assuming the wavelength.""" |
There was a problem hiding this comment.
- Not clear to me what L0 is without context. Can you explain in the docstrings? Either here or module-level?
- Could a helper from
scippneutronbe used for this conversion?
| return { | ||
| **gg, | ||
| 't0': t0_estimate, | ||
| 'tof': lambda time_of_arrival, t0: time_of_arrival - t0, |
There was a problem hiding this comment.
Can you clarify why we still have tof here despite the switch to use the new wavelength-LUT in other code above?
There was a problem hiding this comment.
Beer currently does not use the LUT approach to compute wavelengths. Eventually we will move to that approach for the pulse shaping modes (the modulation modes are incompatible with the LUT approach).
There was a problem hiding this comment.
But I saw a lot of changes that replaced Tof with Wavelength?
There was a problem hiding this comment.
Yes, the workflow is still using the generic unwrap workflow which does not have any public Tof... types.
There was a problem hiding this comment.
So where does TOF come into play? Is it TOA->TOF->Wavelength, or TOA->Wavelength->TOF as for DREAM final result, or something else?
There was a problem hiding this comment.
It's toa->tof->wavelength.
There was a problem hiding this comment.
So what is GenericUnwrapWorkflow being used for? As far as I understand it does not support TOF anymore.
There was a problem hiding this comment.
It's used so that the interface of the workflow matches what it will look like once we have lookup tables for the pulse skipping modes. That the wavelengths are currently not computed using lookup tables is an implementation detail, it does not need to be reflected in the public interface.
2 participants
Fixes #235