BUG: Fix array_api arrays not inplace adding neg zeros correctly
#21212
Conversation
|
Are we sure we want to tape this over here and not look into why this is the case in NumPy proper? I would need to dig deeper to figure out which SIMD branches are used here (and whether this is expected, not expected, or we can do something about it easily). |
|
Ah, so the reason is actually the following:
So effectively, NumPy calculates In principle, it seems like the truly correct thing would be to define all sums using Until then, a quick fix may be to avoid the reduction path for this type of input (usually at least). That won't fix |
|
Ah interesting consequence of a
Yep this PR is certainly not ideal. I'm pretty ignorant about NumPy internals—so if avoiding the reduction path is simple I'll need a pointer (have no idea where to look), otherwise I might not be your guy 😅 |
|
@honno if you like a bit of digging. I think another good start (and good enough for this), would be to make sure that The other thing would be to modify the numpy/numpy/core/src/umath/fast_loop_macros.h Line 102 in e2467e4 I do think both may make sense independently, although the second is just an implementation detail, and the first is a broader fix, since it probably fixes |
|
I'm in agreement that we should fix the underlying numpy function. If that ends up being impossible right now, I'd rather just leave this as is and XFAIL the array-api-tests test. |
Technically, we should ensure that we do all summations starting with -0 unless there is nothing to sum (in which case the result is clearly 0). This is a start, since the initial value is still filled in as 0 by the reduce machinery. However, it fixes the odd case where an inplace addition: x1 = np.array(-0.0) x2 = np.array(-0.0) x1 += x2 May go into the reduce code path (becaus strides are 0). We could avoid the reduce path there, but -0 here is strictly correct anyway and also a necessary step towards fixing `sum([-0., -0., -0.])` which still requires `initial=-0.0` to be passed manually right now. There are new `xfail` marked tests also checking the path without initial. Presumably, we should be using -0.0 as initial value, but 0.0 as default (if an array is empty) here. This may be more reasonably possible after numpygh-20970. Closes numpygh-21213, numpygh-21212
|
Closed as #21218 resolves this problem much better |
Technically, we should ensure that we do all summations starting with -0 unless there is nothing to sum (in which case the result is clearly 0). This is a start, since the initial value is still filled in as 0 by the reduce machinery. However, it fixes the odd case where an inplace addition: x1 = np.array(-0.0) x2 = np.array(-0.0) x1 += x2 May go into the reduce code path (becaus strides are 0). We could avoid the reduce path there, but -0 here is strictly correct anyway and also a necessary step towards fixing `sum([-0., -0., -0.])` which still requires `initial=-0.0` to be passed manually right now. There are new `xfail` marked tests also checking the path without initial. Presumably, we should be using -0.0 as initial value, but 0.0 as default (if an array is empty) here. This may be more reasonably possible after numpygh-20970. Closes numpygh-21213, numpygh-21212
3 participants
Fixes #21211 and includes a regression test. Testing for neg zeros is a bit annoying—let me know if I missed an existing utility for this.
Note I opted to just fallback on
ndarray.__add__(). This is not ideal as we lose the checks against an inplace array having their dtype/shape modified, so I hard-coded those checks in (an introduced an additional test).