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

@Antetokounpo

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...

@mhvk

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

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...

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?

@iamsoto

@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.

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__.

Still an issue

Add scouting report for Issue astropy#10776

@mhvk

I had a look at this and wanted to check the intended direction before writing any code, since @mhvk noted above that it needs some thought.

Confirming it still reproduces on astropy 8.0.1 / numpy 2.4.6:

>>> q = 10 * u.one
>>> dt = TimeDelta(1., format='jd')
>>> q.__array_ufunc__(np.multiply, '__call__', q, dt)
<Quantity 10.>          # expected NotImplemented

The TimeDelta is dropped silently. It shows up more clearly with a dimensional quantity, where the day unit just disappears:

>>> qm = 10 * u.m
>>> qm.__array_ufunc__(np.multiply, '__call__', qm, dt)
<Quantity 10. m>        # vs.  qm * dt  ->  <Quantity 10. d m>

The operators still give the right answer, because Time and TimeDelta set __array_priority__ = 20000 against Quantity's 10000, so __rmul__ wins before __array_ufunc__ is ever consulted.

The origin looks like this line in converters_and_unit:

units = [getattr(arg, "unit", None) for arg in args]

Anything without a .unit becomes None and is then treated as dimensionless.

Where I got stuck is the discriminator. My first thought was to check whether an argument defines its own __array_ufunc__, but that does not work: Time and TimeDelta define none at all, and are not ndarray subclasses. Measured on 8.0.1:

arg	`.unit`	`__array_ufunc__`	`__array_priority__`	ndarray subclass
`ndarray`	no	ndarray's	ndarray default	yes
`list`, `float`	no	absent	none	no
`Quantity`	yes	own	10000	yes
`Time`, `TimeDelta`	no	absent	20000	no

From Quantity's point of view the only thing separating a TimeDelta from a plain list is __array_priority__, which is the mechanism numpy would rather people moved away from. So I do not think keying on it is right, but I also cannot see a clean alternative that does not amount to an explicit opt-in.

Two questions before I go further:

Is the intended rule roughly "no .unit and not coercible to a plain numeric array means NotImplemented", with ndarrays, scalars and sequences still allowed through as dimensionless? Or were you thinking of something more explicit, along the lines of the opt-in marker the issue description hints at, where a class can say it has a unit attribute that Quantity should not assume it understands?
Should the check live in converters_and_unit, or earlier in Quantity.__array_ufunc__ before converters are computed? You mentioned keeping the path for other quantities fast, and I would rather not add work to the common two-Quantity case.

Happy to implement whichever shape you prefer, with tests covering Time and TimeDelta plus a plain third-party class. I did not want to guess at the semantics on something that touches every Quantity operation.

@Dhevenddra

@Dhevenddra - your analysis is correct. Possibly the easiest approach for deciding whether or not something is compatible with Quantity is just try to convert it to it. In principle, there are two places where one can act: in converters_and_unit or in the loop over inputs in Quantity.__array_ufunc__ itself. I think one thing we can do is to re-use astropy.units.core._condition_arg which is called inside a regular converter as well, and checks that the input is numerical or array_like in the right way.

If we do that in __array_ufunc__ it might look like:

            # Same for inputs, but here also convert if necessary.
            arrays = []
            for input_, converter in zip(inputs, converters):
                if converter is None and not isinstance(input_, np.ndarray):
                    # Ensure input is numerical array-like.
                    converter = _condition_arg
                input_ = getattr(input_, "value", input_)
                arrays.append(converter(input_) if converter else input_)

Does this make sense, and would you be interested in trying it out? Note that in principle, this could also be done inside converters_and_unit, but there isn't a suitable loop over inputs already there (and really, it doesn't quite make as much sense, as it should really just deal with units...).

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

astrojuanlu commented on Oct 6, 2020

mhvk commented on Oct 6, 2020

i-am-b-soto commented on Oct 12, 2020

mhvk commented on Oct 12, 2020

mhvk commented on Oct 13, 2020

mhvk commented on Nov 4, 2025

Dhevenddra commented on Jul 29, 2026

mhvk commented on Aug 23, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions