Closed SimonHeybrock closed 8 months ago
I had a short look but I think it would be much easier for me if we looked at the graphs together and you can explain the different steps, as well as the differences between the different graphs.
I have no further comments here, but are we still waiting to have an in-person discussion about this?
@nvaytet @jl-wynen @jokasimr I'd like some input here, on the latest changes.
Also, maybe I've asked this before, but why is this notebook in the API reference? It makes it pretty difficult to find in the docs I feel.
It used to be more visible, but was moved there during the switch to the new theme. @jl-wynen can comment on why it is here or where we can move it.
From that perspective the WFM case is just one particular chopper configuration and it's not clear to me why it needs a separate code path.
It does not a different code part, for the most part. But the time-of-flight origin needs to be defined differently, in particular different neutrons within a frame have a different origin.
Green light from me for this round of review.
Do I remember correctly that you said you were going to add tests after this round of review?
@nvaytet Tests updated!
This represents the latest approach to frame unwrapping and WFM (see old #415).
This clearly makes a bunch of assumptions which may turn out to be wrong in practice. At this point I have little confidence that this will be anything close to the final solution (or even that there is a solution at all — in the end we might still be forced to fall back to defining bounds more manually).
Testing is obviously challenging, and given that it is still not clear how all the choppers will be handled, I believe we must iterate on the whole solution (including what @jl-wynen has kicked of with #466) in the coming months.
Notes:
tof.frames
was removed.tof.fakes
, which I hope we can develop further for these and similar tests. This is currently my only idea on how to move forward: Build more realistic fakes, fix the chopper and unwrapping code, iterate.event_time_zero
. This will be essential for experiments with high time-resolution requirements.scippneutron.tof.chopper_cascade
modulescipp.lookup
overscipp.where
andscipp.bin
during unwrapping and WFM stitching (respectively) has equivalent or better performance. I think it is also clearer.Fixes scipp/ess#165.
Follow-up work:
482
483
469