TST: ensure np.equal.reduce raises a TypeError
#21981
Conversation
|
close/reopen |
| # without specifying the dtype | ||
| a = np.array([0, 0]) | ||
| assert_equal(np.equal.reduce(a, dtype=bool), True) | ||
| assert_raises(TypeError, np.equal.reduce, a) |
There was a problem hiding this comment.
This is causing a segfault in the debug builds.
There was a problem hiding this comment.
hmmm sounds like hitting an assert, probably just a wrong one...
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmmmpf, should be able to get a traceback using gdb: https://numpy.org/devdocs/dev/development_environment.html#debugging
But I cannot reproduce with a python3.9 debug version and see nothing with valgrind either. Can you undo the last commit, because I don't know anymore what is running on CI. At least if it is a linux debug print, we can also hook in gdb in CI (although I have to look up how to every time).
If you can reproduce a crash with -g locally, then running gdb will tell us more.
There was a problem hiding this comment.
Oh, I can reproduce, it requires a full test-suite run though... maybe something wrong related to the dispatching caching.
There was a problem hiding this comment.
OK, I think the assert is indeed incorrect and it should be something like:
diff --git a/numpy/core/src/umath/dispatching.c b/numpy/core/src/umath/dispatching.c
index 5aecdd1fc..f2c6deada 100644
--- a/numpy/core/src/umath/dispatching.c
+++ b/numpy/core/src/umath/dispatching.c
@@ -1016,8 +1016,13 @@ promote_and_get_ufuncimpl(PyUFuncObject *ufunc,
signature[i] = (PyArray_DTypeMeta *)PyTuple_GET_ITEM(all_dtypes, i);
Py_INCREF(signature[i]);
}
- else {
- assert((PyObject *)signature[i] == PyTuple_GET_ITEM(all_dtypes, i));
+ else if ((PyObject *)signature[i] != PyTuple_GET_ITEM(all_dtypes, i)) {
+ /*
+ * If signature is forced we may find an incompatible loop that
+ * would be picked by promotion (signature not forced), reject it.
+ */
+ raise_no_loop_found_error(ufunc, (PyObject **)op_dtypes);
+ return NULL;
}
}We still get a slightly different error depending on the code path, but that is because the "normal" error is less precise.
EDIT: It seems without the assert we already end up in a check which correctly rejects the loop later (not quite sure where).
There was a problem hiding this comment.
OK, maybe not the ideal thing, but I pushed a second commit that applies that fix (with a slightly different comment) and adds a more explicit test.
There was a problem hiding this comment.
Thank you for fixing this, @seberg!! I was caught up with something and couldn't undo the commit (thanks for fixing that too!) :)
If we cache a promoted version of the loop, that promoted can mismatch the correct one. This ends up being rejected later in the legacy paths (should be the only path currently used), but we should reject it here (or in principle we could reject it after cache lookup, but we are fixing up the operation DTypes here anyway, so we are looking at the signature). A call sequence reproducing this directly is: np.add(1, 2, signature=(bool, int, None)) # should fail np.add(True, 2) # A promoted loop np.add(1, 2, signature=(bool, int, None)) # should still fail Not that the errors differ, because the first one comes from the old type resolution code and is currently less precise
a355379 to
e3bcb6a
Compare
np.equal.reduce raises a TypeErrornp.equal.reduce raises a TypeError
Closes #20929
Added a couple of tests to ensure that
np.equal.reduceraisesTypeError, and the developers are notified if the API changes.I am not very sure if I should be creating a new method for this test. Please let me know if I can add the tests in any existing method.
Thanks!