Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Make ndarray inplace operators forward calls when needed. #9021

Merged
merged 1 commit into from
Apr 29, 2017

Conversation

charris
Copy link
Member

@charris charris commented Apr 29, 2017

For backward compatibility, we need to let __array_priority__
determine the override for inplace ops. If __array_ufunc__ = None
on the right hand side it is still the case that TypeError will be
raised.

This fixes SciPy test failures of the type

ndarray += sparse_array

Closes #9019.

@eric-wieser
Copy link
Member

Needs documentation changes too - this broken behaviour was explicitly documented

@njsmith
Copy link
Member

njsmith commented Apr 29, 2017

So I think the conclusion of that discussion was specifically that if the RHS object sets __array_ufunc__ = None, then the ndarray in-place operators should raise an error and not return NotImplemented. OTOH obviously for existing classes doing stuff with __array_priority__ then we have to preserve the old behavior. [Edit: and hopefully we can someday reach a world where the in-place operator methods never return NotImplemented at all, but that's a whole other process.]

If I'm reading right, I think that the __array_ufunc__ PR fixed the new cases but broke the old ones, and that currently this PR reverses that by fixing the legacy cases and breaking the new ones :-)

@charris
Copy link
Member Author

charris commented Apr 29, 2017

@eric-wieser Yeah, documentation still needs fixes. No new tests are needed after the small fix in this PR. Probably should add a note there.

@charris
Copy link
Member Author

charris commented Apr 29, 2017

@njsmith Sounds like we need an INPLACE_GIVE_UP_IF_NEEDED macro to make the distinction.

@charris
Copy link
Member Author

charris commented Apr 29, 2017

Thinking about it, we should probably raise a deprecation warning. Sparse (and others), really should not expect it to work with ndarrays because Python does the wrong thing there.

@charris charris force-pushed the fix-inplace-operators branch 2 times, most recently from 2357509 to c612792 Compare April 29, 2017 17:03
@charris
Copy link
Member Author

charris commented Apr 29, 2017

OK, fixed up so that in-place ops forward on __array_priority__ if the right hand side does not define __array_ufunc__. Still need documentation.

I'm going to wait on documentation until #9014 goes in so as to avoid conficts.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuous integration to the rescue! But all looks OK to me (modulo a space).

return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.add);
}

static PyObject *
array_inplace_subtract(PyArrayObject *m1, PyObject *m2)
{
INPLACE_GIVE_UP_IF_NEEDED(
m1, m2, nb_inplace_subtract,array_inplace_subtract);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trivial fix: space after ,

For backward compatibility, we need to let `__array_priority__`
determine the override for inplace ops. If `__array_ufunc__ = None`
on the right hand side it is still the case that TypeError will be
raised.

This fixes SciPy test failures of the type

   ndarray += sparse_array

Closes numpy#9019.
@charris
Copy link
Member Author

charris commented Apr 29, 2017

@mhvk Thanks for the quick review.

@charris charris merged commit af54fbd into numpy:master Apr 29, 2017
@charris charris deleted the fix-inplace-operators branch April 30, 2017 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scipy sparse inplace fails after __array_ufunc__ merge
4 participants