Move the tof lookup table error threshold to the GenericTofWorkflow#313
Move the tof lookup table error threshold to the GenericTofWorkflow#313
Conversation
| "pulse_stride", | ||
| "distance_resolution", | ||
| "time_resolution", | ||
| "error_threshold", |
There was a problem hiding this comment.
Note that the table may or may not have a error_threshold coord. If it does not, trying to remove it using drop_coords would raise. So we filter it out here.
|
|
||
| # Default parameters | ||
| wf[PulseStrideOffset] = None | ||
| wf[LookupTableRelativeErrorThreshold] = 1.0 |
There was a problem hiding this comment.
Should the default be something like np.inf?
| ErrorLimitedTofLookupTable, | ||
| LookupTableRelativeErrorThreshold, | ||
| MonitorLtotal, | ||
| PulseStrideOffset, | ||
| TimeOfFlightLookupTable, |
There was a problem hiding this comment.
Do we worry about inconsistent naming, TimeOfFlightLookupTable vs LookupTableRelativeErrorThreshold etc.?
There was a problem hiding this comment.
Do you mean should LookupTableRelativeErrorThreshold be changed to TofLookupTableRelativeErrorThreshold?
As it's sort of a new param we are adding to the GenericTofWorkflow, now would probably be the best time to make the change.
As for TimeOfFlightLookupTable vs TofLookupTable, this has already been changed and the old TimeOfFlightLookupTable is kept as an alias for retro-compatibility.
There was a problem hiding this comment.
My view for a long has been the we should scope types in their modules instead of having a generic types + "import everything". But in the end it does not matter, your choice.
| # TODO: The error threshold could be made dependent on the time-of-flight or | ||
| # distance, instead of being a single value for the whole table. | ||
| da = table.array | ||
| relative_error = sc.stddevs(da.data) / sc.values(da.data) |
There was a problem hiding this comment.
Is da.data guaranteed to be positive?
There was a problem hiding this comment.
The data will either be time-of-flight or wavelength, so yes they will always be positive.
|
|
||
| # Default parameters | ||
| wf[PulseStrideOffset] = None | ||
| wf[LookupTableRelativeErrorThreshold] = 1.0 |
There was a problem hiding this comment.
I don't know, it seems to have worked fine for the 3 examples (v20, dream, ordin) that I tried.
But as I mention above in a comment, I was wondering if it should just be Inf as a default value...
2 participants
See discussion here: scipp/essdiffraction#236 (comment)
Instead of hard-coding the error threshold on the LUTs saved to disk, we mask later, in the
GenericTofWorkflow.The
LookupTableRelativeErrorThresholdis now a parameter of the mainGenericTofWorkflowinstead of the workflow that builds the table, and can be set by the user.This means that scientists can control what threshold they want to apply.
We set a default value on the workflow so that we don't break existing workflows and notebooks.