Update ufunc override to work properly with ufunc methods. #4626

cowlicks · 2014-04-19T04:39:36Z

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 inputs and kwargs. But what is an input and what is a kwarg?

The methods I'm wondering about are ufunc.outer(A, B) and ufunc.at(a, indices[, b]).
For outer I guess both A and B should be in inputs? For at should b also be in inputs.

Also please check my reference counting, I'm not confident about it.

cowlicks · 2014-04-19T19:26:02Z

This fixes the bug mentioned here.

charris · 2014-04-20T23:51:02Z

numpy/core/src/umath/ufunc_object.c

@@ -4939,6 +4949,20 @@ ufunc_at(PyUFuncObject *ufunc, PyObject *args)
    char * err_msg = NULL;
    NPY_BEGIN_THREADS_DEF;

+    /* override vars */
+    int errval;


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.

charris · 2014-04-20T23:59:40Z

Bunch of warnings like this

numpy/core/src/private/ufunc_override.h:270:16: warning: comparison with string literal results in unspecified behavior [-Waddress]

Might be related to the almost universal test failures.

When you are ready, could you clean up the commit history and messages? Messages like Better tests for ufunc.method overrides. are not very informative. There should be a description of what is done and what problem it solves.

charris · 2014-04-23T17:47:22Z

All compiled and ran, but all had failures.

juliantaylor · 2014-04-23T18:10:56Z

numpy/core/src/private/ufunc_override.h

-    method_name = PyUString_FromString(method);
-    if (method_name == NULL) {
+    /* ufunc.reduce */
+    else if (strncmp(method, "reduce", 6) == 0) {


this also catches "reduceat"
maybe an enum is more appropriate

cowlicks · 2014-04-23T19:31:05Z

@juliantaylor good catch, I'll do that when I get a chance.

@charris I'll rebase with the appropriate BUG/ENH formatting once I get all these failures sorted out.

cowlicks · 2014-04-30T01:53:43Z

Any clue about the segfault?
https://travis-ci.org/numpy/numpy/jobs/24039989

cowlicks · 2014-04-30T02:57:11Z

numpy/core/src/private/ufunc_override.h

+        else {
+            /* keepdims */
+            PyDict_SetItemString(*normal_kwds, "keepdims", obj);
+        }


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.

seberg · 2014-04-30T09:54:01Z

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)? valgrind --suppressions=python.supp command. You can actually get some hint on finding dangling references using valgrind --suppressions=python.supp --track-origins=yes --leak-check=yes --show-possibly-lost=no command. I bet there are better ways though... You can try sebastian.sipsolutions.net/python.supp for a suppressions file (it should remove a lot of spam due to how python allocates stuff).

juliantaylor · 2014-04-30T17:18:39Z

numpy/core/src/umath/ufunc_object.c

@@ -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);


kwds is uninitialized here, probably the cause of the segfault

Yep, that was it. Thanks!

now theres a memory leak, why not simply pass in NULL?

Done, thanks.

cowlicks · 2014-05-01T04:00:16Z

@charris Rebased with proper commit messages.

charris · 2014-05-04T14:36:49Z

@pv Does this LGTY.

pv · 2014-05-04T14:44:37Z

numpy/core/tests/test_umath.py

+        assert_equal(res[4], (1, a))
+        assert_equal(res[5], {})
+
+        res = np.multiply.reduce(a, 'axis0', 'dtype0', 'out0', 'keep0')


No tests with kwargs --- these check just that the positional args work ok.

Working on it. I implemented at with the last arg as a kwarg which is incorrect.

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

—
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

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

@pv kwargs tests added.

are the non reduce kwargs tested? extobj, where, casting etc?

charris · 2014-05-04T20:04:50Z

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.

juliantaylor · 2014-05-15T17:28:57Z

the non-reduce kwargs cannot be passed positionally, so its probably fine
I'm going to merge it today

juliantaylor · 2014-05-15T21:49:56Z

merging, thanks

Update ufunc override to work properly with ufunc methods.

charris reviewed Apr 20, 2014
View reviewed changes

juliantaylor reviewed Apr 23, 2014
View reviewed changes

cowlicks reviewed Apr 30, 2014
View reviewed changes

juliantaylor reviewed Apr 30, 2014
View reviewed changes

pv reviewed May 4, 2014
View reviewed changes

cowlicks added 3 commits May 9, 2014 16:40

BUG: Properly parse ufunc methods with PyUFunc_CheckOverride.

6fdee1c

BUG: Fix and add ufunc override capabilities to ufunc methods.

d261913

TST: Add tests for ufunc.method overrides.

c8ac77c

juliantaylor added a commit that referenced this pull request May 15, 2014

Merge pull request #4626 from cowlicks/ufunc-override-methods

003fcda

Update ufunc override to work properly with ufunc methods.

juliantaylor merged commit 003fcda into numpy:master May 15, 2014

cowlicks deleted the ufunc-override-methods branch May 16, 2014 02:13

pv mentioned this pull request Jun 2, 2014

BUG difference in behaviour for subclass output in ufuncs #4753

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ufunc override to work properly with ufunc methods. #4626

Update ufunc override to work properly with ufunc methods. #4626

cowlicks commented Apr 19, 2014

cowlicks commented Apr 19, 2014

charris Apr 20, 2014

charris commented Apr 20, 2014

charris commented Apr 23, 2014

juliantaylor Apr 23, 2014

cowlicks commented Apr 23, 2014

cowlicks commented Apr 30, 2014

cowlicks Apr 30, 2014

seberg commented Apr 30, 2014

juliantaylor Apr 30, 2014

cowlicks May 1, 2014

juliantaylor May 2, 2014

cowlicks May 3, 2014

cowlicks commented May 1, 2014

charris commented May 4, 2014

pv May 4, 2014

cowlicks May 9, 2014

njsmith May 9, 2014

pv May 9, 2014

cowlicks May 9, 2014

juliantaylor May 9, 2014

charris commented May 4, 2014

juliantaylor commented May 15, 2014

juliantaylor commented May 15, 2014

Update ufunc override to work properly with ufunc methods. #4626

Update ufunc override to work properly with ufunc methods. #4626

Conversation

cowlicks commented Apr 19, 2014

cowlicks commented Apr 19, 2014

Choose a reason for hiding this comment

charris commented Apr 20, 2014

charris commented Apr 23, 2014

Choose a reason for hiding this comment

cowlicks commented Apr 23, 2014

cowlicks commented Apr 30, 2014

Choose a reason for hiding this comment

seberg commented Apr 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowlicks commented May 1, 2014

charris commented May 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

- assert_equal(res, "reduce")

- assert_equal(res, "accumulate")

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented May 4, 2014

juliantaylor commented May 15, 2014

juliantaylor commented May 15, 2014