MAINT: Ensure ufunc override call each class only once, plus cleanup #11338

mhvk · 2018-06-14T21:17:12Z

Replaces #11320, for which I wrote:
"This started as a simple fix for #11306, ensuring that as we collect arguments and overrides, we skip those that have the same class as ones collected already. But looking at the code (mostly my own...), I found a reference counting bug (first commit), and also realized that for the ndarray side, where all that is needed is to know whether there are any overrides, the code was needlessly slow. But splitting that out, there was no need any more for much of the code to be in the src/private directory, so I moved it. As a result, this is quite a lot of shuffling, but I think it makes the whole a lot clearer and cleaner.

Left really is the annoyance of not having access to the PyArray API - thus, this should be revisited once multiarray and umath have been merged."

This PR follows @eric-wieser's request to make the commits more stand-alone, and in particular ensure that the one that moves functions around does nothing else than the move. This is true here (well, except that the names of the functions were changed).

~~Note that this is based on #11336 - I'll rebase once that is merged.~~ EDIT: now done.

EDIT: also kept out the move for now, for easier review

fixes #11306

eric-wieser · 2018-06-14T22:39:29Z

^ to avoid needing a rebase

eric-wieser · 2018-06-14T22:40:02Z

numpy/core/src/private/ufunc_override.c

-        PyObject *method;
+        PyObject *obj;
+        int j;
+        npy_bool new_class = 1;


NPY_TRUE, if you're using npy_bool

yes, maybe I shouldn't bother

eric-wieser · 2018-06-14T22:45:58Z

numpy/core/src/private/ufunc_override.c

@@ -106,7 +105,10 @@ PyUFunc_WithOverride(PyObject *args, PyObject *kwds,
    }

    for (i = 0; i < nargs + nout_kwd; ++i) {
-        PyObject *method;
+        PyObject *obj;
+        int j;


This can move into the if (with_override ...) block - no need to declare it here

In a later commit, it ends up in the right place.

eric-wieser · 2018-06-14T22:49:02Z

numpy/core/src/private/ufunc_override.c

         */
-        method = get_non_default_array_ufunc(obj);
-        if (method != NULL) {
+        if (with_override != NULL) {


What about when with_override == NULL? Won't things get called twice? When is it NULL?

No, methods and with_override are either both given or both NULL. This is partially why I ended up taking the function apart - it just didn't make sense to try to make one function work both for collecting and checking overrides. (It is not that great to come back to one's own code and find it rather poorly thought out!)

eric-wieser · 2018-06-14T22:49:32Z

Having the move commit be part of this PR at all makes the comments above all appear as outdated...

mhvk · 2018-06-15T01:24:56Z

Shall I split the PR in two parts? I realize the move indeed makes it very unhandy, also to change things, since I effectively have to do it twice when I rebase.

eric-wieser · 2018-06-15T01:45:19Z

I think that would be best.

eric-wieser · 2018-06-17T23:44:53Z

numpy/core/src/private/ufunc_override.c

            }
        }
+        else {
+            nout = 1;


Wouldn't it be better to just cast to a 1-tuple here? We already do that when we call __array_wrap__ and __array_ufunc__

Could also do:

PyObject **out_objs; if (out_kwd_obj == NULL) { nout = 0; out_objs = NULL; } else if (PyTuple_CheckExact(out_kwd_obj)) { nout = PySequence_Fast_GET_SIZE(out_kwd_obj) out_objs = PySequence_Fast_ITEMS(out_kwd_obj); } else { nout = 1 out_objs = &out_kwd_obj; } // Cleanup just xdecrefs out_kwd_obj ``

Could wrap this into

int parse_out_args(PyObject *args, PyObject *kwargs, PyObject **out_ref, PyObject ***out_objs) { // code above return nout }

Which would at least prevent you repeating this logic twice in this file. We can solve the repetitions across files later.

I like that logic very much, and have adapted it; in the end, it should percolate up.

eric-wieser · 2018-06-17T23:51:55Z

numpy/core/src/private/ufunc_override.c

+    if (kwds == NULL) {
+        return 0;
+    }
+    out_kwd_obj = PyDict_GetItemString(kwds, "out");


Not a huge fan of parsing the out argument in yet one more place. Can we use ufunc_full_args somehow?

I'd like to leave that to a later PR (in #11372, it ends being done at the very start of ufunc_generic_call, which I think is most logical, since every part below it needs it).

Fine by me - my objection was to introducing one more parser. I'm happy now that the number of parsers in this file hasn't increased :)

mhvk · 2018-06-18T02:43:35Z

@eric-wieser - thanks for the comments; I'll try to look better later. For now, for fun, have a look at where this is leading: #11372 (I do want to proceed in small steps, but the goal informs some choices, such as not using full_args; more later).

mhvk · 2018-06-18T13:41:19Z

OK, adjusted. Next step here would be to move the stuff that is not shared between umath and multiarray to their respective places (and do a bit more cleanup there).

mattip · 2018-06-21T05:02:26Z

Is this going to be split into two pull reqeusts?

eric-wieser · 2018-06-21T05:39:57Z

numpy/core/src/private/ufunc_override.c

+    *out_kwd_obj = PyDict_GetItemString(kwds, "out");
+    if (*out_kwd_obj == NULL) {
+        return 0;
+        }


nit: indentation

eric-wieser · 2018-06-21T05:41:49Z

numpy/core/src/private/ufunc_override.c

+ * Returns -1 if kwds is not a dict, 0 if no outputs found.
+ */
+static int
+get_out_objects(PyObject *kwds, PyObject **out_kwd_obj, PyObject ***out_objs)


I don't think you need out_kwd_obj as an argument - you don't seem to use it

But I need a place to store the reference - at least, I think I do, and when I tried initially without it, it definitely had random segfaults (I considered initializing out_objs with a NULL reference in its first slot, but I felt that it was better not to write a function that expected such initialized array was a bit much). But perhaps I am just missing something? (C remains not one of my strengths...)

Oh, I get it now - otherwise you end up returning a reference to a local variable. That's pretty unfortunate.

eric-wieser · 2018-06-21T05:43:46Z

numpy/core/src/private/ufunc_override.c

-                        "Internal Numpy error: call to PyUFunc_HasOverride "
-                        "with non-tuple");
-        goto fail;
+    nin = PyTuple_Size(args);


This is misleading - it's not the number of inputs, but the number of positional args, some of which may be outputs

Actually, I'm not sure if that's true - if it's not, can you clarify that PyUFunc_WithOverride never receives out arguments positionally?

You are right, it is just the positional arguments. This means that for reductions arguments like axis get checked too, which is not great, but out of scope here (there is an issue, #9107, and I do hope to get to it...)

Anyway, now clarified this and called things arg instead of in.

eric-wieser · 2018-06-21T05:47:32Z

@mattip: Looks sufficiently split to me, but some more comments

mhvk · 2018-06-21T12:27:21Z

@mattip - it was already split up; there will be a follow-up PR to move some of the functions to umath and multiarray, since they are no longer shared (but that movement made review tricky).

mhvk · 2018-06-21T12:51:21Z

@eric-wieser - addressed your smaller comments; if you have a suggestion for how to deal with out_kwd_obj, do let me know.

mhvk · 2018-06-27T18:40:06Z

@eric-wieser - shall we get this in? I'd like to get to the second part of this...

eric-wieser · 2018-06-28T07:22:29Z

numpy/core/src/private/ufunc_override.c

+    PyObject **in_objs, **out_objs;
+
+    /* check inputs */
+    nin = PyTuple_Size(args);


nit: PySequence_FAST_SIZE would be more consistent

Nevermind, what you have behaves a little better in the face of bad input

eric-wieser · 2018-06-28T07:24:09Z

numpy/core/src/private/ufunc_override.c

+/*
+ * Check whether any of a set of input and output args have a non-default
+ *  `__array_ufunc__` method. Return 1 if so, 0 if not.
+ */


I think this comment needs an explanation of why this function needs to exist in addition to PyUFunc_WithOverride (ie, performance, maybe?)

eric-wieser · 2018-06-28T07:28:57Z

I'd like to see a little more in-source explanation of why both WithOverride and HasOverride are needed. It wasn't immediately obvious to me that Has is:

primarily for optimization
can short-circuit unlike With
Isn't O(nin^2) now that duplicates are checked for in With

mhvk · 2018-06-28T13:43:48Z

OK, done. Note that I didn't mention the O(nin^2), since the check for a class already having been done is much faster than getting the __array_ufunc__ attribute. Indeed, in the original PR, the second half was to move these functions to where they are actually needed, which allows some further optimizations (reflected in the TODO in the comments; since I think this is essentially ready, I'll prepare that follow-up PR now).

Overall, it likely doesn't matter much for performance, but it is more logical and more consistent with what python does: reverse operators are not called if the forward one of a given class already returned NotImplemented.

Code was getting too convoluted and both can be optimized in different ways.

eric-wieser

get_out_objects isn't as nice as I'd hoped, but in future I'd hope that the arguments get parsed earlier anyway, and the need for it goes away

mhvk · 2018-06-28T20:00:13Z

Indeed, eventually get_out_objects should be done very early on.

I'll merge this so I can try my follow-up.

mhvk requested a review from eric-wieser June 14, 2018 21:17

mhvk mentioned this pull request Jun 14, 2018

BUG,MAINT: Ensure ufunc override call each class only once, plus cleanup #11320

Closed

mhvk added 03 - Maintenance component: numpy.ufunc labels Jun 14, 2018

mhvk added this to the 1.16.0 release milestone Jun 14, 2018

eric-wieser changed the base branch from master to maintenance/1.15.x June 14, 2018 22:39

eric-wieser changed the base branch from maintenance/1.15.x to master June 14, 2018 22:39

eric-wieser reviewed Jun 14, 2018

View reviewed changes

mhvk force-pushed the ufunc-override-call-class-only-once-organized branch 2 times, most recently from de4c5ba to f8d034d Compare June 15, 2018 13:02

eric-wieser reviewed Jun 17, 2018

View reviewed changes

mhvk mentioned this pull request Jun 18, 2018

MAINT: Ufunc parsing super fast (work in progress) #11372

Closed

mhvk force-pushed the ufunc-override-call-class-only-once-organized branch from f8d034d to 5cd5590 Compare June 18, 2018 13:40

mhvk force-pushed the ufunc-override-call-class-only-once-organized branch from 5cd5590 to b5f4d8a Compare June 18, 2018 14:06

eric-wieser reviewed Jun 21, 2018

View reviewed changes

mhvk force-pushed the ufunc-override-call-class-only-once-organized branch from b5f4d8a to 8d19718 Compare June 21, 2018 12:50

eric-wieser reviewed Jun 28, 2018

View reviewed changes

mhvk force-pushed the ufunc-override-call-class-only-once-organized branch from 8d19718 to 3d6cc59 Compare June 28, 2018 13:40

mhvk force-pushed the ufunc-override-call-class-only-once-organized branch from 3d6cc59 to 52df1f1 Compare June 28, 2018 13:49

MAINT: Split off getting overrides and checking for them.

4b700ae

Code was getting too convoluted and both can be optimized in different ways.

mhvk force-pushed the ufunc-override-call-class-only-once-organized branch from 52df1f1 to 4b700ae Compare June 28, 2018 13:52

eric-wieser approved these changes Jun 28, 2018

View reviewed changes

mhvk merged commit a6383b4 into numpy:master Jun 28, 2018

mhvk deleted the ufunc-override-call-class-only-once-organized branch June 28, 2018 20:00

mhvk mentioned this pull request Jul 2, 2018

MAINT: move ufunc override code to umath and multiarray as much as possible. #11480

Merged

Uh oh!

MAINT: Ensure ufunc override call each class only once, plus cleanup #11338

MAINT: Ensure ufunc override call each class only once, plus cleanup #11338

Uh oh!

Conversation

mhvk commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Jun 14, 2018

Uh oh!

mhvk commented Jun 15, 2018

Uh oh!

eric-wieser commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 18, 2018

Uh oh!

mhvk commented Jun 18, 2018

Uh oh!

mattip commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Jun 21, 2018

Uh oh!

mhvk commented Jun 21, 2018

Uh oh!

mhvk commented Jun 21, 2018

Uh oh!

mhvk commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 14, 2018 •

edited

Loading

eric-wieser Jun 17, 2018 •

edited

Loading

eric-wieser Jun 17, 2018 •

edited

Loading

eric-wieser Jun 21, 2018 •

edited

Loading

eric-wieser commented Jun 28, 2018 •

edited

Loading

eric-wieser left a comment •

edited

Loading