Fix error when computing mean of array that has unit none - #3874
Conversation
| if (!original_unit.has_value()) | ||
| // temporarily make the unit dimensionless for the math | ||
| num.setUnit(sc_units::one); | ||
| auto result = | ||
| num * reciprocal(astype(denominator, type, CopyPolicy::TryAvoid)); | ||
| result.setUnit(original_unit); // restore None if needed |
There was a problem hiding this comment.
Are you saying we now support mean but not multiplication?
There was a problem hiding this comment.
I am not sure I understood the question.
With these changes, now
a = sc.array(dims=['x'], values=[1, 2, 3, 4], unit=None)
a.mean() # <scipp.Variable> () float64 <no unit> 2.5works.
As before these changes,
a * astill works:
<scipp.Variable> (x: 4) int64 <no unit> [1, 4, 9, 16]
And we still have the same behaviour as before for
b = sc.array(dims=['x'], values=[1, 2, 3, 4], unit="")
a * bwhich gives
---------------------------------------------------------------------------
UnitError Traceback (most recent call last)
Cell In[6], line 2
1 b = sc.array(dims=['x'], values=[1, 2, 3, 4], unit="")
----> 2 a * b
UnitError: Cannot multiply with operand of unit 'None'.
There was a problem hiding this comment.
Ok. But instead of the back and forth with the numerator unit, could we just adjust denominator.setUnit(sc_units::one); to use none if numerator unit is none?
There was a problem hiding this comment.
Yes, sounds good.
I tried that now but I'm running into UnitError: Cannot divide with operand of unit 'None'.
Indeed, if I try 1/a (with a defined above), I get the same error.
Interestingly, if I also try sc.reciprocal(a), I get DTypeError: 'reciprocal' does not support dtypes 'int64', .
If I then change the dtype to float, I am back to the Cannot divide with operand of unit 'None' error.
There was a problem hiding this comment.
We would need to allow reciprocal to work with unit None.
Currently it raises because it tries to compute the reciprocal of the unit 1/unit which is what you want if you have seconds or meters. But if you have unit=None, do you want the unit of 1/a to be None or do we want to continue to forbid this?
In the case of the latter, then I think we can't get around the back and forth with the unit?
There was a problem hiding this comment.
I don't know. To me unit=None always meant something like an memory offset or an index. I don't know what the mean means for that. But I also see little harm in supporting the reciprocal in that case.
Maybe the better definition of unit=None is "treat this as a plain array of values", i.e., we can support all operations for unit=None as long as nothing mixes with unit-full variables.
|
@SimonHeybrock with the current changes, I can do a = sc.array(dims=['x'], values=[1., 2., 3., 4.], unit=None)
sc.reciprocal(a)but I'm guessing because the numerator gets converted to a Variable and then uses |
Surprising but not harmful? The forbid-arithmetic-without-unit decision may anyway be worth revisiting. |
Fixes #3873