Fix unit conversions #187
Fix unit conversions #187
Conversation
…d sample rotation offset twice
a674844 to
bf7a0a2
Compare
nvaytet
left a comment
There was a problem hiding this comment.
I would just convert to float64 everywhere and not worry about the dtypes of the other operands.
We don't always know if L2 or mu will be float (they could also be ints)?
|
Yeah, maybe that's better 🤷 It's a trade-off, we want some flexibility letting users determine data types, without setting up footguns. |
I think this rule is mainly referring to the "large" coords, i.e., the ones we have per-event, rather than per-pixel (or scalars). |
But the reflection angle is used to compute Q and that's a per-event coord. So if we cast the angle to float64 then Q will also always be float64. But I don't mind casting it to float64 instead if you both think that's better. |
Not if you convert to the desired unit before the operation? But probably flat64 everywhere is also fine. |
Do you mean dtype? I don't understand what operation you're referring to. The operation that computes Q? |
Yes.
Yes. |
src/ess/estia/resolution.py
Outdated
| ) | ||
| ).to(unit=theta.unit) | ||
| sc.atan(detector_spatial_resolution.to(unit=L2.unit, dtype='float64') / L2) | ||
| ).to(unit=theta.unit, dtype='float64') |
There was a problem hiding this comment.
This last conversion is operating on a variable which has size Npixels (because of L2).
You could add a copy=False in the call to to(...) in case the units are already the correct ones and we want to return early. That said, Npixels is not that large.
This PR makes some unit conversions safer by explicitly casting their dtypes.
This is important when converting quantities that are provided by users because if the user provides a value with integer dtype the result of the unit conversion can be unexpected.
For example, if the user provides a
SampleRotationOffset: sc.scalar(1, unit='deg')and we convert that toradthen after the conversion the value will be0 rad. By converting the dtype to whatever theSampleRotationdtype is (almost certainly float) we avoid the error.This doesn't solve the problem completely, because strictly speaking we don't distinguish between user provided parameters and non-user provided parameters, but it should prevent errors in most common cases.
Should be rebased on main once #186 is merged.