API: make piecewise subclass safe using use zeros_like.

[mhvk]

Arguably more logical and makes the otherwise very subclass-aware code subclass safe.

Mostly because as I am implementing a new Masked class that can handle subclasses properly, I found that for my __array_function__ helpers I had to copy the whole piecewise implementation to just make this single change.

[seberg]

@rgommers for clarification, how do you think about this type of change considering your proposed NEP? It is unlikely to break anyone's code (but not technically impossible), in this case it was probably intended (using asanyarray). I think we have done this type of change quite often and I don't ever remember issues.

Anyway, this makes sense to me, just needs a test. Release notes and possibly .. versionchanged:: might also be good?

[seberg]

Nevermind, I guess we will just agree this is more of a bugfix, still wonder if ..versionadded and/or release notes would better?

[rgommers]

Nevermind, I guess we will just agree this is more of a bugfix, still wonder if ..versionadded and/or release notes would better?

.. versionchanged:: I think - but not in favour of adding that to the docstring. A release note does seem justified.

[mhvk]

OK, I pushed a changelog entry, renaming the commit message. Obviously, if this does end up causing any trouble, do just revert it. But it does seem just to have been an oversight.

p.s. With this change, it should work with np.ma.MaskedArray as well... I can add a test for that if needed.

[eric-wieser]

I found that for my __array_function__ helpers I had to copy the whole piecewise implementation to just make this single change.

How exactly does this change work? What does a simple __array_function__ look like that uses this codepath?

[mhvk]

@eric-wieser - when implementing __array_function__, one can choose to just forward it to ndarray.__array_function__ with super(). It turns out that for a large fraction of all functions this ducktyping work well enough once one properly covers the ufuncs and basics like shape-changing (e.g., once one does concatenate, all *stack etc work).

Given the huge numpy API, this saves a lot of upfront work when creating a new subclass or duck type. It also means one immediately benefits from any bug fixes in those pieces of code, though of course at the price of the (relatively minor) hassle of fixing things when a version updates changes what happens (it just means covering all 262 wrapped function with tests...).

To get a sense, see the list of "safe" functions at https://github.com/mhvk/astropy/blob/53c06ff40a57b2af577b7877306eaa8a462551a9/astropy/utils/masked/function_helpers.py#L84-L113 for my masked class, and at https://github.com/astropy/astropy/blob/53c06ff40a57b2af577b7877306eaa8a462551a9/astropy/units/quantity_helper/function_helpers.py#L62-L92 for Quantity.
And their use in the implementation of Quantity.__array_function__ at https://github.com/astropy/astropy/blob/53c06ff40a57b2af577b7877306eaa8a462551a9/astropy/units/quantity.py#L1493-L1504

Anyway, concretely with the PR, one more function moves to "safe" - which means I don't have to try to keep a separate copy of the code up to date with that at numpy (as is, probably astropy is already carrying bugs that were solved in numpy; eventually I guess we'd need to move to some type of introspection to get numpy function source code and then apply patches instead of just copying pieces).

[eric-wieser]

one can choose to just forward it to ndarray.__array_function__ with

Ah, I forgot we had that fallback. Thanks!

[seberg]

Hmm, thanks for bringing it up. My first angle was that even without the __array_function__ reasoning this probably just makes sense (whether we like subclasses or not).

I am not sure we really define exactly, what happens when using super().__array_function__(), aside from it going to some existing implementation which may or may not do the correct thing for subclasses. But for better or worse it is a direct exposure of current functionality (as long as all "relevant arguments" are ndarray subclasses).

None of this is ideal, but I expect we have so many other functions fitting into the "safe" category, that one more just doesn't make a difference when it comes to freedom of changing NumPy in the future and at the same time it does help right now.

---

@mhvk one thing I just noticed glancing at the quantity __array_function__ code. You do not seem to check the relevant_args for whether they include only quantity and ndarray? It seems that you can end up calling the NumPy fallback incorrectly if you mix quantity with another subclass (which may or may not know about quantity). In particular, your new masked array + quantity, may do that if quantity is the first in the "left to right" search.

[mhvk]

@seberg - thanks! Indeed, the change would of course also be useful for subclasses that do not define __array_function__! And for the super() call, the logic indeed was that ndarray should let subclasses through, since presumably those behave sufficiently similarly (mostly true for Quantity and Masked...)

On Quantity.__array_function__, I should indeed have a more careful look again as you are correct that as it is, the actual function may be executed even when another ndarray subclass would have needed to do some handling beforehand. The coordination between multiple classes with __array_function__ remains a tricky one...

[seberg]

@mhvk yeah, it is a bit tricky, but mostly if a subclass doesn't define __array_function__ at all (and I am actually not sure it show up in the "relevant_types" then – which is what __array_ufunc__ does). Otherwise, I would expect one of the subclasses explicitly handling the other one and both subclasses to reject anything they do not know. The only issue is that the inherited __array_function__ +may take charge too aggressively, like ndarray.__array_function__ does if you call super() right away.

In theory, there is a general issue for what happens if you inherit. I actually believe that the best way may to have logic like:

if other not in known_types and not issubclass(type(self), other):
    return NotImplemented

The not issubclass(other, type(self)) may look strange, but allows unmodified inheritance (and super() usage), while retaining strict behaviour in case two subclasses inherit: The inherited/super version can take over as long as it is a strict subclass of the other one. (Python itself partially side-steps this by having assymetric __add__ and __radd__, although I think it may be a good pattern also there.)

---

As to this PR, would it be possible to add a short MA (or similar) test? Otherwise I think this is just good to go?

[mattip]

@mhvk this PR is waiting for a test

[mhvk]

@mattip - thanks for the reminder; I added a test now!

[seberg]

LGTM, thanks Marten and matti for the ping :).