ENH: Allow broadcast to be called with zero arguments #13544
Conversation
Follows on from numpygh-6905 which reduced the limit from 2 to 1. Let's go all the way to zero. Just as for `broadcast(broadcast(a), b)` is interpreted as `broadcast(a, b)` , this change interprets `broadcast(broadcast(), a)` as `broadcast(a)`.
2519c53 to
a3a19da
Compare
| @@ -1262,10 +1262,14 @@ PyArray_MultiIterFromObjects(PyObject **mps, int n, int nadd, ...) | |||
| int i, ntot, err=0; | |||
|
|
|||
| ntot = n + nadd; | |||
| if (ntot < 1 || ntot > NPY_MAXARGS) { | |||
| if (ntot < 0) { | |||
There was a problem hiding this comment.
How does one hit the code path where ntot < 0? If there's a concise / practical way to do that, coverage shows this is never true so a test would be useful.
Conversely, if this can't be hit by any practical code path by calling NumPy public API, is the guard really needed?
There was a problem hiding this comment.
I guess the guard is needed because of C API exposure.
| "n and nadd arguments must be non-negative", NPY_MAXARGS); | ||
| return NULL; | ||
| } | ||
| if (ntot > NPY_MAXARGS) { |
There was a problem hiding this comment.
This is never true in the coverage report either, though I suppose a unit test here might be slow if that max value is big.
There was a problem hiding this comment.
I think this entire function is probably untested, like most parts of the c API.
| PyErr_Format(PyExc_ValueError, | ||
| "Need at least 1 and at most %d " | ||
| "array objects.", NPY_MAXARGS); | ||
| "n and nadd arguments must be non-negative", NPY_MAXARGS); |
There was a problem hiding this comment.
Strange message, but not even sure how that should get floated to a user...
There was a problem hiding this comment.
This is part of the c API, so python users will never see it.
| if (PyErr_Occurred()) { | ||
| return NULL; | ||
| } | ||
| if (PyErr_Occurred()) { |
There was a problem hiding this comment.
OK, do you see why this is there? I do not see how there can possibly an error set at this stage. But... It was there before, so I guess we can keep it (and moving it a bit doesn't matter much).
There was a problem hiding this comment.
Yes, isinstance can throw an exception in principle, when it returns -1
There was a problem hiding this comment.
That seems pretty far fetched, but OK. This whole thing is trying to be minimal invasive. And I think that is successful enough for me to be OK with merging it in a bit. (so if anyone has any stomach aches about it, please comment).
The C-API testing would be better of course, so maybe will also see to add that before merging.
Follows on from gh-6905 which reduced the limit from 2 to 1. Let's go all the way to zero.
Just as for
broadcast(broadcast(a), b)is interpreted asbroadcast(a, b), this change interpretsbroadcast(broadcast(), a)asbroadcast(a).Also updates the test to prove that
broadcastis allowed to be called on a single argument.