BUG: __numpy_ufunc__ passes on only a single output argument #4160

mhvk · 2013-12-31T18:46:52Z

I'm trying to use __numpy_ufunc__ for the Quantity subclass in astropy (which adds and propagates units associated with the array), and while mostly it is a joy compared to the previous __array_prepare__ etc tangle, for functions with two outputs there is a problem, in that only one output is passed on among the keyword arguments. This can be seen from the following example:

import numpy as np
class trial(np.ndarray):               
    def __new__(cls, *args, **kwargs):
        return np.array(*args, **kwargs).view(cls)
    def __numpy_ufunc__(self, ufunc, method, i, inputs, **kwargs):
        print(self, ufunc, method, i, inputs, kwargs)
        inputs = [arg.view(np.ndarray) for arg in inputs]
        return ufunc(*inputs, **kwargs)

a = trial(100)
b = np.array([1.])
c = np.array([2.])
np.modf(a, b)
#100 <ufunc 'modf'> __call__ 0 (trial(100),) {'out': array([ 1.])}
# (array([ 0.]), array([ 100.]))
np.modf(a, b, c)
#100 <ufunc 'modf'> __call__ 0 (trial(100),) {'out': array([ 2.])}
# (array([ 0.]), array([ 100.]))

The text was updated successfully, but these errors were encountered:

njsmith · 2013-12-31T21:31:46Z

This looks like a pretty serious bug in __numpy_ufunc__, since it makes it impossible for modf and other multiple-output ufuncs to work correctly with any class defining a __numpy_ufunc__ method when out= is used, and causes wrong results to be returned silently. (It's not even possible to fall back on the default implementation, because the way __numpy_ufunc__ works its the job of the called method to do this, and when this bug kicks in then the arguments are already lost before it can delegate.) I'm going to tentatively mark it as a 1.9 blocker.

CC: @pv, @cowlicks

pv · 2014-01-01T01:31:48Z

Yep, 1.9 blocker. The case if multiple output args is ignored in the implementation. How to deal with them is also unspecified in the spec.

This is probably simple to fix, however, now that we are not bound by any backward compatibility constraints.

mhvk · 2014-01-01T01:45:54Z

@pv - the docstrings of both np.modf and np.frexp start with

modf(x[, out1, out2])

This would suggest one possible choice, of having out1 and/or out2 among kwargs.

Note, though, that one cannot give these as keyword arguments in the direct call either. While np.modf(a, b, c) and np.modf(a, out=b) work, np.modf(a, out=b, out2=c) or anything equivalent does not; rather, it raises

ValueError: cannot specify 'out' as both a positional and keyword argument

ps. while trying the above, I realised the test on whether something is a keyword argument is simply whether it starts with "out"; np.modf(100.5, outblabla=a) does fine.

njsmith · 2014-01-01T02:09:13Z

Iirc the clearer ufunc calling convention is out=(arr1, arr2), so that's
what we'd want to normalize to for numpy_ufunc.

The outblahblah thing sounds like a bug too, but a different one - maybe
file it separately?
On 31 Dec 2013 19:45, "Marten van Kerkwijk" notifications@github.com
wrote:

@pv https://github.com/pv - the docstrings of both np.modf and np.frexpstart with

modf(x[, out1, out2])

This would suggest one possible choice, of having out1 and/or out2 among
kwargs.

Note, though, that one cannot give these as keyword arguments in the
direct call either. While np.modf(a, b, c) and np.modf(a, out=b) work, np.modf(a,
out=b, out2=c) or anything equivalent does not; rather, it raises

ValueError: cannot specify 'out' as both a positional and keyword argument

ps. while trying the above, I realised the test on whether something is a
keyword argument is simply whether it starts with "out"; np.modf(100.5,
outblabla=a) does fine.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4160#issuecomment-31417092
.

cowlicks · 2014-01-02T14:43:49Z

So in the PyUFunc_CheckOverride function we implicitly assume there is only one output here:

    /* If we have more args than nin, the last one must be `out`.*/
    if (nargs > nin) {
        obj = PyTuple_GET_ITEM(args, nargs - 1);
        PyDict_SetItemString(normal_kwds, "out", obj);
    }

I think what we actually want to do is grab all the positional arguments after the ninth one and pass them like out = (arr1, arr2, ...) (as @njsmith suggests) in the kwargs dict.

I'll try this out and get back to y'all.

edit: override was supposed to be kwargs

cowlicks · 2014-01-04T17:38:39Z

Here is how I propose we handle output variables:

one positional output variable x is passed in the kwargs dict as out : x
multiple positional output variables x0, x1, ... are passed as a tuple in the kwargs dict as out : (x0, x1, ...)
keyword output variables like out = x and out = (x0, x1, ...) are passed unchanged to the kwargs dict like out : x and out : (x0, x1, ...) respectively.
combinations of positional and keyword output variables are not supported.

Note that this does not change the proposed interface. Does this sound okay? cc @njsmith @pv

cowlicks · 2014-01-04T19:20:26Z

See pull 4171.

pv · 2014-01-08T18:35:02Z

Fixed in gh-4171

mhvk mentioned this issue Dec 31, 2013

Monkey-patch non-ufunc Numpy functions astropy/astropy#1274

Closed

pv closed this as completed Jan 8, 2014

mhvk mentioned this issue May 18, 2014

Transition Quantity to use __array_ufunc__ astropy/astropy#2492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

BUG: __numpy_ufunc__ passes on only a single output argument #4160

BUG: __numpy_ufunc__ passes on only a single output argument #4160

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BUG: __numpy_ufunc__ passes on only a single output argument #4160

BUG: __numpy_ufunc__ passes on only a single output argument #4160

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!