Conversation
|
Hi! Your PR was missing some labels 🔖 so I added them: essreduce |
| and event_time_offset in the lookup table. | ||
| """ | ||
| distance_unit = "m" | ||
| time_unit = "us" |
There was a problem hiding this comment.
why not ns, since event_time_offset typically has unit ns?
There was a problem hiding this comment.
It was because scientists usually think about time-of-flight in μs, and so when looking at lookup tables, it make more sense visually when the axis is in μs.
That said, you are right that this means we are almost always making a copy of the array of event_time_offset because we comvert it to μs...
I think I will use ns and save the compute 👍
There was a problem hiding this comment.
Aha I see, in that case either unit works for me
There was a problem hiding this comment.
Hmm actually it's the other way round:
we take the event_time_offset unit: https://github.com/scipp/ess/blob/fast-lut/packages/essreduce/src/ess/reduce/unwrap/to_wavelength.py#L270
and convert the axis of the LUT to that unit: https://github.com/scipp/ess/blob/fast-lut/packages/essreduce/src/ess/reduce/unwrap/to_wavelength.py#L93
| min_dist = ltotal_range[0].to(unit=distance_unit) | ||
| max_dist = ltotal_range[1].to(unit=distance_unit) | ||
|
|
||
| # We need to bin the data below, to compute the weighted mean of the wavelength. |
There was a problem hiding this comment.
What does this refer to? I don't see any binning or weighted mean here. Was the comment duplicated from the other lookup-table-computation function?
There was a problem hiding this comment.
Yes, copied over from old computation.
| # Find the correct simulation reading | ||
| selected_frame = None | ||
| for frame in sorted_frames: | ||
| if dist.value >= frame.distance.to(unit=dist.unit).value: |
There was a problem hiding this comment.
I don't quite understand this, what is frame here? Why are there multiple of them? Later on in this function we compute "subframes" associated with the selected frame, what are those?
There was a problem hiding this comment.
The 'frame' and 'subframe' terminology is from the chopper_cascade module: https://scipp.github.io/scippneutron/user-guide/chopper/chopper-cascade.html
A frame sequence contains frames, which is a description of how the pulse looks like at different stages along the beamline. There is a frame at the source (when the pulse is just a rectangle), one frame at each chopper, and a final frame that was propagated to the detector distance.
Then, each frame consists of one or multiple Subframe which are the polygons on the figure. Basically, one Frame is depicted by one colour on the figure. And there are one or more polygons of the same colour.
| pulse_period=pulse_period, | ||
| pulse_stride=pulse_stride, | ||
| distance_resolution=table.coords["distance"][1] - table.coords["distance"][0], | ||
| time_resolution=table.coords["event_time_offset"][1] |
There was a problem hiding this comment.
Is there a reason why we are not using the time_resolution and distance_resolution arguments here?
There was a problem hiding this comment.
The final time_resolution may not be exactly what the value in the argument was, because we want it to fit exactly into one or more pulse periods. We give the step size which is the closest to what the user requested but fits exactly in the range (resolution will always be at least what the user requested or finer, never coarser).
The distance_resolution is preserved and in principle we could just use the argument. In this case, it is the ltotal_range which is not exactly preserved. But in case we decided later to change that behaviour in the future, I wanted to be sure that bugs were not introduced here (like forgetting to update the resolution in the table).
So I thought the safest is just to take it from the array.
| # around the frame period, so we cover two pulses strides. | ||
| frequency_for_chopper_rotation = (1.0 / pulse_period.to(unit='s')) / ( | ||
| pulse_stride * 2 | ||
| ) |
There was a problem hiding this comment.
I'd prefer if pulse_stride was not an argument to this function, because essentially the pulse stride is a function of the chopper system, and we already pass the chopper parameters as an argument. Can we derive the pulse_stride from the chopper parameters?
There was a problem hiding this comment.
The initial thought was that trying to guess from chopper settings was dangerous.
Were you thinking of guessing from the rotation frequencies?
There may be chopper configurations we did not think of?
For example, can there be a chopper that instead of having one opening and spins at 14 Hz, it has 2 symmetric openings and spins at 7 Hz?
Or maybe during hot commissioning, they may operate the source at 7Hz, meaning that Odin might just decide to park the pulse skipping chopper, meaning that we find only choppers with frequencies of 14 Hz and higher, thus guessing a pulse_stride of 1?
There was a problem hiding this comment.
Yes I was thinking we could guess from the chopper frequencies.
Is it a problem if we guess a "too large" pulse stride?
My understanding is that it's only a problem if we guess a "too small" pulse stride. So in the case of a 7Hz chopper with two openings wouldn't be a problem, because we would guess pulse_stride=2, while it could have been treated as 1 as well.
I don't really understand the Odin example, if all choppers are operating at 14 Hz then the
| return 0.5 * (y_min + y_max), 0.5 * (y_max - y_min) | ||
|
|
||
|
|
||
| def _compute_mean_wavelength_in_polygons( |
There was a problem hiding this comment.
Maybe rename to _estimate_wavelength_by_polygon_centers to be more specific.
|
|
||
|
|
||
| @dataclass | ||
| class SourcePulse: |
There was a problem hiding this comment.
Maybe rename to UniformSourcePulse to make it distinct from the non-uniform source pulse that is used in tof.
There was a problem hiding this comment.
How about SourceExtent or SourceBounds?
I agree SourcePulse isn't great, but I'm not sure users will immediately understand why Uniform comes into play?
Alternative to #557:
Instead of replacing the existing workflow with the one that uses
scippneutron.tof.chopper_cascadeto compute the wavelength LUT, we simply add a new workflow.This can then be rolled out to different packages as desired.
The next step will be to use the providers from the new workflow and inject them directly into the
GenericUnwrapWorkflowto compute the LUT on-the-fly. We defer that to a later PR.Additional info (copied from #557)
To approximate the polygons as single lines, we produce a set of vertical lines that represent the bins in
event_time_offset.For each line, we find the intersections with the polygon edges, and then compute the midpoint between the min and max y coordinate of the intersections.
The table below is computed in ~0.7s compared to ~16s before (using 5M neutrons).
To use:
Needs scipp/scippneutron#700