Skip to content

Conversation

@juliantaylor
Copy link
Contributor

See commit messages for some details.

  • intern the constant strings, was accidentally not done in the original PR.
  • avoid a __numpy_ufunc__ string attribute lookup on pure python types which are very expensive
  • don't use keyword arguments for the reduction wrappers np.sum etc. Non keyword arguments parse a lot faster with the regular python function. This is an alternative to ENH: speed up ufunc reduce argument parsing #4174 which improves keyword parsing in general but has higher complexity. The keyword arguments one of the major reasons np.sum is so much slower than np.add.reduce (4us vs 2.5 us).

The last point fails a __numpy_ufunc__ test which I think is a bug in the logic of the override.
The override assumes all non input and output arguments are passed in via keyword arguments and then uses the ufunc number of inputs to consider all following arguments as output.
The last commit breaks both assumptions, out is not provided via a keyword and the reduction only has one input and not two as the non reduction types.
The effect is that you get a tuple [dtype, out, keepdims] as the out argument of the ufunc (axis is missing as nin is not treated right for reduce).

This looks to me like a bug, but I'm not very familiar with the override, please advise.

@juliantaylor
Copy link
Contributor Author

@pv @cowlicks comments on the override issue?

@cowlicks
Copy link
Contributor

This does appear to be bug with __numpy_ufunc__. When reduce is called we check for __numpy_ufunc__ and pass the number of inputs to PyUfunc_CheckOverride as ufunc->nin, which is incorrect. See here we have:

static PyObject *
ufunc_reduce(PyUFuncObject *ufunc, PyObject *args, PyObject *kwds)
{
    int errval;
    PyObject *override = NULL;

    errval = PyUFunc_CheckOverride(ufunc, "reduce", args, kwds, &override, 
                                   ufunc->nin);
    if (errval) {
        return NULL;
    }
    else if (override) {
        return override;
    }
    return PyUFunc_GenericReduction(ufunc, args, kwds, UFUNC_REDUCE);
}

@charris
Copy link
Member

charris commented Jan 15, 2014

Redo Travis, IIRC there have been some __numpy_ufunc__ fixes.

@seberg
Copy link
Member

seberg commented Jan 22, 2014

If there have been fixes, I think it may need to be rebased to run the new tests?

@charris
Copy link
Member

charris commented Jan 23, 2014

Well, it still fails...

@charris
Copy link
Member

charris commented Jan 30, 2014

Restarted Travis tests.

@charris
Copy link
Member

charris commented Jan 30, 2014

Could use a rebase, Yeah.

@charris
Copy link
Member

charris commented Jan 30, 2014

OK, checked a rebased version and

ERROR: test_multiarray.TestBinop.test_ufunc_override_rop_simple
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/charris/Workspace/numpy.git/build/testenv/lib64/python2.7/site-packages/numpy/core/tests/test_multiarray.py", line 1746, in test_ufunc_override_rop_simple
    assert_equal(obj2.sum(), 42)
  File "/home/charris/Workspace/numpy.git/build/testenv/lib64/python2.7/site-packages/numpy/core/_methods.py", line 32, in _sum
    return umr_sum(a, axis, dtype, out, keepdims)
  File "/home/charris/Workspace/numpy.git/build/testenv/lib64/python2.7/site-packages/numpy/core/tests/test_multiarray.py", line 1715, in __numpy_ufunc__
    r = func(*inputs, **kw)
TypeError: output must be an array

Looks fixable.

@cowlicks
Copy link
Contributor

cowlicks commented Apr 1, 2014

What is the correct way to get the number of arguments for ufunc reductions? I'm looking for something like ufunc.nin but for the reduction methods.

If there isn't currently a way to do this. Is there a precise definition of nin which we could use to make a way? I'm wondering about a case like numpy.multiply.reduce(arr, ax), should the axis argument add to nin?

@cowlicks
Copy link
Contributor

cowlicks commented Apr 1, 2014

In PyUFunc_CheckOverride we assume that if there are more arguments than ufunc.nin then they must be out arguments, here. This doesn't work for reduction methods that take positional axis args since these are not counted in ufunc.nin.

So I think we'll have to add a special case for each reduction method so its arguments get parsed correctly.

@njsmith
Copy link
Member

njsmith commented Apr 1, 2014

reduce and accumulate both exist only if nin==2, and they always take
exactly 1 array argument.

outer I guess acts like call

reduceat IIRC will always take 2 array arguments, and only exists for
nin==2, so I guess the current logic might work by accident but it would
probably be better to handle it explicitly anyway for clarity :-)

On Tue, Apr 1, 2014 at 10:58 PM, Blake Griffith [email protected]:

In PyUFunc_CheckOverride we assume that if there are more arguments than
ufunc.nin then they must be out arguments, herehttps://github.com/numpy/numpy/blob/master/numpy/core/src/private/ufunc_override.h#L98.
This doesn't work for reduction methods that take positional axis args
since these are not counted in ufunc.nin.

So I think we'll have to add a special case for each reduction method so
its arguments get parsed correctly.


Reply to this email directly or view it on GitHubhttps://github.com//pull/4186#issuecomment-39265463
.

Nathaniel J. Smith
Postdoctoral researcher - Informatics - University of Edinburgh
http://vorpus.org

PyArg_ParseTupleAndKeywords is pretty slow for keywords as it needs to
create python strings first. Using positional arguments avoids this and
gains 15-20% performance for small reductions.
@juliantaylor
Copy link
Contributor Author

this can now go in, I think its still useful for 1.9 to fit the theme of faster small array operations

@charris
Copy link
Member

charris commented May 15, 2014

LGTM, thanks Julian.

charris added a commit that referenced this pull request May 15, 2014
Improve small reduction performance
@charris charris merged commit e8e40dc into numpy:master May 15, 2014
@juliantaylor juliantaylor deleted the reduce-opt branch November 24, 2014 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants