Drop index from __numpy_ufunc__ signature? out always a tuple?

@njsmith

This is a copy of #4753 (comment) by @njsmith in #4753, which I think has broader scope than the PR there:

OK, going to say something unpopular. (Certainly I'm annoyed at myself for saying it, but nonetheless...) I've been debating this with myself for a while, and I've come to the conclusion that we should bite the bullet and drop the index argument from numpy_ufunc's signature. This will require some care to avoid breaking already released versions of scipy.sparse (lesson learned: we should never have released scipy.sparse with numpy_ufunc before it came out in numpy), but that's better than leaving it forever.

Rationale:

the index argument is never useful. The only time it's useful at all is for classes that want to cast self to ndarray and recurse, and even in those cases they can just as well do a loop and handle all objects of their type at once. It'll be faster too. None of the example classes we've written during the binop discussion have used it at all.
the impossibility of having a useful value for output arguments makes there index even more useless. (Sure we can set it to n > nin, but what's the use of this? Writing code to check for this and do something sensible is more complicated than just finding the array by hand.)
there are almost certainly other arguments we should dispatch on, like where=. What do we set the index to in this case?

Sorry. I'm the one who suggested adding it in the first place, but it was a bad idea :-/

Also while we're at it, we should make the out= argument unconditionally a tuple, rather than sometimes a single ndarray and sometimes a tuple of them. This would simplify argument processing (see: all the times we wrote if not isinstance(out, tuple): out = (out,) on the example binop implementations).

Thanks!
On Jun 19, 2015 1:51 PM, "Marten van Kerkwijk" [email protected]
wrote:

This is a copy of #4753 (comment)
#4753 (comment) by
@njsmoth in #4753 #4753, which I
think has broader scope than the PR there:

OK, going to say something unpopular. (Certainly I'm annoyed at myself for
saying it, but nonetheless...) I've been debating this with myself for a
while, and I've come to the conclusion that we should bite the bullet and
drop the index argument from numpy_ufunc's signature. This will require
some care to avoid breaking already released versions of scipy.sparse
(lesson learned: we should never have released scipy.sparse with
numpy_ufunc before it came out in numpy), but that's better than
leaving it forever.

Rationale:

the index argument is never useful. The only time it's useful at all
is for classes that want to cast self to ndarray and recurse, and even in
those cases they can just as well do a loop and handle all objects of their
type at once. It'll be faster too. None of the example classes we've
written during the binop discussion have used it at all.

the impossibility of having a useful value for output arguments
makes there index even more useless. (Sure we can set it to n > nin, but
what's the use of this? Writing code to check for this and do something
sensible is more complicated than just finding the array by hand.)

there are almost certainly other arguments we should dispatch on,
like where=. What do we set the index to in this case?

Sorry. I'm the one who suggested adding it in the first place, but it was
a bad idea :-/

Also while we're at it, we should make the out= argument unconditionally a
tuple, rather than sometimes a single ndarray and sometimes a tuple of
them. This would simplify argument processing (see: all the times we wrote
if not isinstance(out, tuple): out = (out,) on the example binop
implementations).

—
Reply to this email directly or view it on GitHub
#5986.

Now not specifically to #4753: indeed, I never used the index at all in my trials with Quantity, since I always have to check the other arguments anyway. It also definitely would be nice if out always were a tuple; this would make it more consistent with inputs being one as well. And I agree that eventually we probably want to add where arguments as the ones that can override np.ufunc.

But I don't know how we can do it: sadly, scipy.sparse actually uses the index: https://github.com/scipy/scipy/blob/master/scipy/sparse/base.py#L819.

@njsmith

I'm putting all the blockers as 1.10.release at this point.

@njsmith Will this change break anything in released Scipy? I believe it checks for __numpy_ufunc__ support rather than being version dependent, so is probably OK.

@pv Thoughts?

@njsmith

If we make sure that scipy.sparse objects continue to get priority for
binop methods, then this won't break anything in released scipy.
We have to do this for mul no matter what, so it's not a big deal
I think.

(Analysis: in current scipy.sparse, the binop methods do their
thing, and numpy_ufunc either dispatches to the relevant binop
method or attempts to perform the actual ufunc. Historically, ndarray

sparse and sparse + ndarray have worked, but ufunc(sparse) has not.
If we make this change to numpy_ufunc, then ufunc(sparse) will not
work with released versions of scipy -- but it never did, so this
isn't a regression. So the only risk is if ndarray + sparse started
calling sparse.numpy_ufunc instead of sparse.add. So we just
have to make sure that doesn't happen.)

On Sun, Jun 21, 2015 at 1:55 PM, Charles Harris
[email protected] wrote:

I'm putting all the blockers as 1.10.release at this point.

@njsmith Will this change break anything in released Scipy? I believe it
checks for numpy_ufunc support rather than being version dependent, so
is probably OK.

@pv Thoughts?

—
Reply to this email directly or view it on GitHub.

Nathaniel J. Smith -- http://vorpus.org

+1 to both these suggestion.

Indeed, doing a loop over the array arguments and using isinstance(arr, type(self)) or arr is self is simpler and more robust. index simply leads uninformed users astray (such as myself during my first attempt to write a __numpy_ufunc__ method).

It would have been useful to have this discussion earlier, but things always happen at their own pace. . +0 on removing the index argument and +1 for making out always a tuple. . However, I know this change will break at cvxopt at the least, and maybe other things also. There is code out there that calls ufuncs directly on scipy sparse matrices + object arrays. . The only transition path I see forward is: (i) release point versions of Scipy without the numpy_ufunc code removed ASAP, and (ii) when the release of Numpy with numpy_ufunc is released, the release notes need to contain a warning that it is incompatible with Scipy versions < XXX.

We do also have the option of renaming the method, which would definitely
avoid all problems. I don't like it because I can't think of any other
names that are nearly as good, but sometimes we have to suck it up and deal
with such things.
.
To make sure I understand the issue, though: what happens with numpy master

latest scipy when you call 'ufunc(sparse, object array)'? It converts
sparse into an object array and then re-calls the ufunc?
On Jun 23, 2015 12:54 AM, "Pauli Virtanen" [email protected] wrote:

It would have been useful to have this discussion earlier, but things
always happen at their own pace.
.
+0 on removing the index argument and +1 for making out always a tuple.
.
However, I know this change will break at cvxopt at the least, and maybe
other things also. There is code out there that calls ufuncs directly on
scipy sparse matrices + object arrays.
.
The only transition path I see forward is: (i) release point versions of
Scipy without the numpy_ufunc code removed ASAP, and (ii) when the
release of Numpy with numpy_ufunc is released, the release notes need to
contain a warning that it is incompatible with Scipy versions < XXX.

—
Reply to this email directly or view it on GitHub
#5986 (comment).

By analogy with the old __array_prepare__, __array_wrap__ construct, maybe __array_ufunc__?

As we're considering changes, one hopefully small one that I'd really like to see, is for ndarray to have __numpy_ufunc__ defined, so ndarray subclasses can super it. (This will be different from calling the function directly in that it is possible to get back NotImplemented, which can then be passed on.)

__array_ufunc__ sounds pretty solid to me, especially given how generic (numpy agnostic) all of the implementations we've seen so far are.

For example, numba has its own DUFunc object that works like a ufunc but has delayed call signature generation (so you don't have to prespecify dtypes). This object should work fine with __numpy_ufunc__, too.

@mhvk

@mhvk: It's true -- see #6011. I filed this as a separate bug because while it's definitely a good idea, it's a missing feature that can be added later, while the changes to the name/signature discussed in this thread are things we have to sort out ASAP.