Quantity.__array_ufunc__ should return NotImplemented for Time instances (and other classes that it does not know about)

[mhvk]

In #10653 (comment), @Antetokounpo found that if Time.__array_ufunc__ is defined, timedelta * dimensionless_quantity fails to work correctly. This is because Quantity.__array_ufunc__ does not correctly return NotImplemented:

from astropy.time import TimeDelta
from astropy import units as u
dt = TimeDelta(1., format='jd')
q = 10*u.one
q.__array_ufunc__(np.multiply, '__call__', q, dt)
# should return NotImplemented
# <Quantity 10.>

Right now, this bug is hidden by the fact that Time does not define __array_ufunc__, which means that __array_priority__ is used, which is higher than Quantity.__array_priority__ and hence q * dt gets done by TimeDelta.__rmul__.

EDIT: this is a more general problem also for any class that defines a unit attribute but would like Quantity not to assume it understands it. Understanding should be opt-in somehow...

[astrojuanlu]

@mhvk do you think this can be labeled as "good first issue"?

[mhvk]

Hmm, not sure. It does need a bit of thought. In particular, to ensure the path of other quantities remains fast, i.e., it cannot just be an upfront check that any of the arguments is a Time. It would also help if whoever does it has some sense of how __array_ufunc__ actually works... I worry that "good first issue" just means that one of us ends up doing it anyway...

[i-am-b-soto]

So I took a look at Quantity.__array_ufunc__

The problem appears to be this method takes objects with a non-defined 'unit' property and ultimately treats them as dimensionless_unscaled.
Without re-writing the logic to check for specific objects in array_ufunc, a simple workaround might be to give TimeBase a property 'unit' that raises NotImplemented ?

How does the team feel about such an approach?

[mhvk]

@Iamsoto - it would work, but I feel it puts the burden in the wrong place - really, Quantity.__array_ufunc__ shouldn't let something that doesn't have a unit and that it doesn't know about it through. So, my sense is that it would be better to do some updates to __array_ufunc__ and perhaps units.quantity_helpers.converters.converters_and_unit.

p.s. The present implementation is a bit too keen on duck typing, I fear. Generally, duck typing is a good thing, but not for things where one deals with the interaction of multiple classes, all of which might be able to make things work.

[mhvk]

Actually, thinking more about this: what would also solve this is if Time and TimeDelta were to have __array_ufunc__, for which there are some efforts underway (#10272; see also #10688).

EDIT (2021-jan-5): that does not help, of course, that is the whole issue! So, will need to be smarter in Quantity.__array_ufunc__.