LokiWorkflow with time-of-flight calculation #223
Conversation
|
This is good, it's what I was going to start with to implement a tof workflow for Loki 👍 |
…eringRunType as it did not play well with the tof workflow
nvaytet
changed the title
…h events stripped
|
@SimonHeybrock I would like you to review but I can't select you as a reviewer since you opened the PR... |
src/ess/loki/workflow.py
Outdated
| TransmissionRun, | ||
| ) | ||
|
|
||
| bank_size = {'layer': 4, 'tube': -1, 'straw': 7, 'pixel': 512} |
There was a problem hiding this comment.
Why did you choose to make tube the free param? If I remember correctly, pixel was the one that they could configure freely.
There was a problem hiding this comment.
You wrote that part 😉 (5cdbd72)
I think it's because all banks have 7 straws, and 4 layers, but they don't all have the same number of tubes.
I can look up the number of tubes for each bank and then set the pixel to -1 if you think that is more future-proof?
src/ess/sans/types.py
Outdated
| # class TofDetector(sciline.Scope[RunType, sc.DataArray], sc.DataArray): | ||
| # """Data with a time-of-flight coordinate""" | ||
|
|
||
|
|
||
| class TofMonitor(sciline.Scope[RunType, MonitorType, sc.DataGroup], sc.DataGroup): | ||
| """Monitor data with a time-of-flight coordinate""" | ||
| # class TofMonitor(sciline.Scope[RunType, MonitorType, sc.DataGroup], sc.DataGroup): | ||
| # """Monitor data with a time-of-flight coordinate""" |
| # For simplicity, insert a fake beam center instead of computing it. | ||
| wf[BeamCenter] = sc.vector([0.0, 0.0, 0.0], unit='m') | ||
| wf[NeXusDetectorName] = f'loki_detector_{bank}' | ||
| with pytest.raises(UnsatisfiedRequirement, match='TimeOfFlightLookupTableFilename'): |
There was a problem hiding this comment.
Makes me wonder once again why you chose not to abbreviate these types. We use Tof elsewhere, so this you be TofLookupTableFilename, or even just TofLUTFilename. But of course this is unrelated to this PR, so ignore!
There was a problem hiding this comment.
Do you know if it would be possible to change the name without breaking everything downstream?
E.g. by adding an alias TimeOfFlightLookupTableFilename = TofLookupTableFilename?
There was a problem hiding this comment.
Yes that should work, only confusing bit might be what shows up in visualizations, but I suppose that would be temporary.
| "source": [ | ||
| "## Setting up the workflow\n", | ||
| "\n", | ||
| "Note here that for no, we have no chopper in the beamline.\n", |
There was a problem hiding this comment.
| "Note here that for no, we have no chopper in the beamline.\n", | |
| "Note here that for now, we have no chopper in the beamline.\n", |
| "wf[Filename[SampleRun]] = loki.data.loki_coda_file_one_event()\n", | ||
| "\n", | ||
| "# Visualize the workflow\n", | ||
| "wf.visualize(BackgroundSubtractedIofQ, graph_attr={'rankdir': 'LR'})" |
There was a problem hiding this comment.
| "wf.visualize(BackgroundSubtractedIofQ, graph_attr={'rankdir': 'LR'})" | |
| "wf.visualize(IntensityQ, graph_attr={'rankdir': 'LR'})" |
since you only set the sample run above, so background subtraction wouldn't work?
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "wf.compute(RawDetector[SampleRun])" |
There was a problem hiding this comment.
Can't we at least compute tof and wavelength? Also, direct beam was optional iirc, and transmission can be computed from monitors?
There was a problem hiding this comment.
In the end, I use the same file for all runs, and compute the IntensityQ.
It's all NaNs at the end, but the workflow still runs.
SimonHeybrock
left a comment
SimonHeybrock
left a comment
There was a problem hiding this comment.
LGTM (cannot approve).
This is not complete, put together quickly to assist with Loki detector tests.Expanded the
LokiWorkflowto use theGenericTofWorkflow.The Larmor workflow still uses the old simple conversion
tof = event_time_offset.We remove the
ScatteringRunTypeas it is not compatible with theGenericTofWorkflowwhich computeTofDetector[RunType]instead ofTofDetector[ScatteringRunType].The workflow seems to be happy without it.
Needs: scipp/essreduce#292