Thanks!
On Jun 19, 2015 1:51 PM, "Marten van Kerkwijk" notifications@github.com
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.

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?

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
notifications@github.com 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).

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" notifications@github.com 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: 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.

@mhvk @pv @njsmith Four days is a long time, can we move this forward. I'm agnostic on the index change and using a new, renamed function. What does the rename route imply for third party extensions?

What does the rename route imply for third party extensions?

Third party extensions that already have a release with __numpy_ufunc__ (scipy.sparse and astropy?) will need to start over by implementing __array_ufunc__ with the new signature.

There are no backwards compatibility concerns, because nobody currently calls __numpy_ufunc__. They can simply rename __numpy_ufunc__ to __array_ufunc__ after tweaking the signature to the new spec.

The main implication is that scipy will need to make another release before making using of this.

Just answering the practical question: No issue for astropy -- I have PRs with the __numpy_ufunc__ implementation (astropy/astropy#2583; astropy/astropy#2948) but they aren't merged yet so can be trivially changed.

Concur with @shoyer and @mhvk's analyses.

I think we have consensus here and the changes are easy; the only reason I haven't done them yet is that they'll conflict with #6001 so I'm waiting for that to get resolved first.

Ping here to revisit apropos #8247.

This was done in #8247