Update ufunc override to work properly with ufunc methods. #4626
Conversation
|
This fixes the bug mentioned here. |
| @@ -4939,6 +4949,20 @@ ufunc_at(PyUFuncObject *ufunc, PyObject *args) | |||
| char * err_msg = NULL; | |||
| NPY_BEGIN_THREADS_DEF; | |||
|
|
|||
| /* override vars */ | |||
| int errval; | |||
There was a problem hiding this comment.
Need to declare before NPY_BEGIN_THREADS_DEF;, which has an empty statement before; and makes this declaration after a code statement. This is an error on windows also.
|
Bunch of warnings like this Might be related to the almost universal test failures. When you are ready, could you clean up the commit history and messages? Messages like |
|
All compiled and ran, but all had failures. |
| method_name = PyUString_FromString(method); | ||
| if (method_name == NULL) { | ||
| /* ufunc.reduce */ | ||
| else if (strncmp(method, "reduce", 6) == 0) { |
There was a problem hiding this comment.
this also catches "reduceat"
maybe an enum is more appropriate
|
@juliantaylor good catch, I'll do that when I get a chance. @charris I'll rebase with the appropriate |
|
Any clue about the segfault? |
| else { | ||
| /* keepdims */ | ||
| PyDict_SetItemString(*normal_kwds, "keepdims", obj); | ||
| } |
There was a problem hiding this comment.
Do I need to Py_DECREF(obj) here (and other places)? I'm not sure if PyDict_SetItemString has its own reference to obj now.
|
Don't know that much debugging foo, but did you try running the test in valgrind (best selectively just that file with runtests maybe, it is slow)? |
| @@ -4939,6 +4954,15 @@ ufunc_at(PyUFuncObject *ufunc, PyObject *args) | |||
| char * err_msg = NULL; | |||
| NPY_BEGIN_THREADS_DEF; | |||
|
|
|||
| /* `nin`, the last arg, is unused. So we put 0. */ | |||
| errval = PyUFunc_CheckOverride(ufunc, "at", args, kwds, &override, 0); | |||
There was a problem hiding this comment.
kwds is uninitialized here, probably the cause of the segfault
There was a problem hiding this comment.
Yep, that was it. Thanks!
There was a problem hiding this comment.
now theres a memory leak, why not simply pass in NULL?
|
@charris Rebased with proper commit messages. |
|
@pv Does this LGTY. |
| assert_equal(res[4], (1, a)) | ||
| assert_equal(res[5], {}) | ||
|
|
||
| res = np.multiply.reduce(a, 'axis0', 'dtype0', 'out0', 'keep0') |
There was a problem hiding this comment.
No tests with kwargs --- these check just that the positional args work ok.
There was a problem hiding this comment.
Working on it. I implemented at with the last arg as a kwarg which is incorrect.
There was a problem hiding this comment.
What state is master in if this doesn't get merged? Would leaving this out
of 1.9 mean that we release 1.9 with a broken numpy_ufunc API and get
stuck in some compatibility trap?
On Fri, May 9, 2014 at 8:34 PM, Blake Griffith notifications@github.comwrote:
In numpy/core/tests/test_umath.py:
- assert_equal(res, "reduce")
res = np.multiply.accumulate(a, 1)- assert_equal(res, "accumulate")
res = np.multiply.reduceat(a, 1)assert_equal(res, "reduceat")res = np.multiply.**call**(1, a)assert_equal(res[0], a)assert_equal(res[1], np.multiply)assert_equal(res[2], '**call**')assert_equal(res[3], 1)assert_equal(res[4], (1, a))assert_equal(res[5], {})
res = np.multiply.reduce(a, 'axis0', 'dtype0', 'out0', 'keep0')Working on it. I implemented at with the last arg as a kwarg which is
inco 8000 rrect.—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4626/files#r12492498
.
Nathaniel J. Smith
Postdoctoral researcher - Informatics - University of Edinburgh
http://vorpus.org
There was a problem hiding this comment.
In master: if ufuncs are given extra optional arguments as positional arguments --- apart from the input and out= one --- they are stuffed to out kwarg instead of separate kwargs in master.
While this is buggy in itself, it is in practice perhaps not a common situation. (As the ufunc extra args are obscure stuff, people tend to call them as kwargs.)
There was a problem hiding this comment.
@pv kwargs tests added.
There was a problem hiding this comment.
are the non reduce kwargs tested? extobj, where, casting etc?
|
I'm going to branch 1.9.x today, and wait a few days for 1.9.0. This can be backported if it is ready in time. |
|
the non-reduce kwargs cannot be passed positionally, so its probably fine |
|
merging, thanks |
Update ufunc override to work properly with ufunc methods.
This allows the ufunc override machinery to work properly with all the ufunc methods.
There is one failing test here which I am looking into. It passes for some python versions and not others.
The ufunc methods required some API design decisions and I'd like to hear what people think. The original ufunc override spec called for the arguments to be parsed into
inputsandkwargs. But what is an input and what is a kwarg?The methods I'm wondering about are
ufunc.outer(A, B)andufunc.at(a, indices[, b]).For
outerI guess bothAandBshould be ininputs? Foratshouldbalso be ininputs.Also please check my reference counting, I'm not confident about it.