Raise errors if we are trying to access values outside of the distance and pulse range of the time-of-flight lookup table #211
Conversation
| ltotal: sc.Variable, | ||
| event_time_offset: sc.Variable, | ||
| ) -> sc.Variable: | ||
| if pulse_index.unit != sc.units.dimensionless: |
There was a problem hiding this comment.
Shouldn't it have unit == None?
There was a problem hiding this comment.
We could... it comes from (event_time_zero - reference) // pulse_period. So it's kind of dimensionless, but it's true that it doesn't represent a physical quantity.
It would add some code like manually setting pulse_index.unit = None, which currently is not needed.
I don't mind either way.
There was a problem hiding this comment.
We had a similar discussion at some point in ESSreflectometry. If the index is used in mathematical operations with non-index quantities, then it is easiest to stick with dimensionless. Otherwise, I'd switch to unit=None.
There was a problem hiding this comment.
How about having if pulse_index.unit not in ('', None): so that we can decide to change it to None later and it won't break here?
Currently, we are using numpy/scipy to perform the interpolation, so the unit doesn't really matter.
But we may decide to change to using Scipp in the future.
| f"pulse_index must be dimensionless, but got unit: {pulse_index.unit}." | ||
| ) | ||
| if ltotal.unit != self._distance_unit: | ||
| raise ValueError( |
There was a problem hiding this comment.
| raise ValueError( | |
| raise sc.UnitError( |
?
And the same for the other cases.
Errors are now raised if we attempt to compute tof outside of the
distanceandpulserange of the lookup table.We do not raise for the
event_time_offsetrange (even though it should all wrap around 71ms) because histogrammed monitors often have binning which can be anything (does not necessarily stop at 71ms).Raising an error here would be too restrictive, and warnings would add noise to the workflows.