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

mhvk · 2018-06-13T00:46:31Z

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.

fixes #11306

shoyer · 2018-06-13T01:09:03Z

numpy/core/src/umath/override.c

Would it be better to store types in an array rather than calling PyObject_Type repeatedly?

OOPS, this is the wrong function, I don't need a reference to the type at all, I just need Py_TYPE, which is very fast.

I think this is wrong elsewhere in the function too...

OK, I think I got them all.

eric-wieser · 2018-06-13T02:16:05Z

numpy/core/src/multiarray/methods.c

nit: remove a layer of parens here

eric-wieser · 2018-06-13T02:16:23Z

numpy/core/src/multiarray/methods.c

Shouldn't be comparing with NULL - this returns a boolean

duh, how could I have missed all but one of those.

eric-wieser · 2018-06-13T02:16:56Z

numpy/core/src/multiarray/methods.c

has_override shouldn't be an npy_bool if it contains -1

Indeed. Leftover. I do wonder to what extent I should even check that args is a tuple and kwds a dict; one can presumably only get here via python, so this should be OK. What do you think? remove those tests?

Could write them as asserts

Good idea, done in array_ufunc itself.

eric-wieser · 2018-06-13T02:18:58Z

numpy/core/tests/test_umath.py

Test might be clearer and more reorderable if you did c.count = c_sub.count = 0 each time (or just recreated the arrays)

OK, does make more sense. Done.

eric-wieser · 2018-06-13T02:20:23Z

numpy/core/src/private/ufunc_override.c

A comment explaining the purpose of this would be nice

Something like /* Like the python `__op__` and `__rop__` overloads, a type is not visited more than once */

eric-wieser · 2018-06-13T02:21:34Z

First commit is good to go, only minor nits on the second.

eric-wieser · 2018-06-13T02:40:43Z

Would prefer to see the move commit separately - it makes it harder to review (reviewing per-commit results in comments being marked outdated) and rebase with it part of the same PR.

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.

This way, only one simple routine is left.

Where we args is guaranteed to be a tuple, and were out is useful immediately.

eric-wieser · 2018-06-13T02:42:45Z

numpy/core/src/private/ufunc_override.c

                goto fail;
            }
            if (with_override != NULL) {
+                for (j = 0; j < num_override_args; j++) {


The int j can go on the line above this - there's no need to introduce it 70 lines earlier

mhvk · 2018-06-13T02:52:10Z

Makes sense about the review, though perhaps it is not so bad to check what is here directly...
I'll look tomorrow whether I can perhaps organize it better.

Bug since the former increases reference counts, which were not taken into account.

mhvk · 2018-06-14T21:18:02Z

Replaced by #11338 - which has the commits better organized (and likely solves the failure due to a poor assert statement).

BUG: also decref with_override in failure path.

e5a75e1

mhvk added 00 - Bug 03 - Maintenance component: numpy.ufunc labels Jun 13, 2018

mhvk requested a review from eric-wieser June 13, 2018 00:46

shoyer reviewed Jun 13, 2018

View reviewed changes

eric-wieser reviewed Jun 13, 2018

View reviewed changes

numpy/core/src/multiarray/methods.c Outdated

Copy link

Member

eric-wieser Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove a layer of parens here

eric-wieser reviewed Jun 13, 2018

View reviewed changes

mhvk added 4 commits June 12, 2018 22:41

MAINT: Split off getting overrides and checking for them.

4bf43fd

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

MAINT: move out all non-shared code from ufunc_override.

d5d3c4a

This way, only one simple routine is left.

MAINT: cleanup umath/override.c

ed190ca

Where we args is guaranteed to be a tuple, and were out is useful immediately.

mhvk force-pushed the ufunc-override-call-class-only-once branch from d9068e6 to a22b713 Compare June 13, 2018 02:41

eric-wieser reviewed Jun 13, 2018

View reviewed changes

mhvk force-pushed the ufunc-override-call-class-only-once branch from a22b713 to 16c39af Compare June 13, 2018 02:49

mhvk force-pushed the ufunc-override-call-class-only-once branch 2 times, most recently from fa2c834 to 2ac7463 Compare June 13, 2018 12:37

BUG: replace incorrect PyObject_Type with Py_TYPE.

56b0d57

Bug since the former increases reference counts, which were not taken into account.

mhvk force-pushed the ufunc-override-call-class-only-once branch from 2ac7463 to 56b0d57 Compare June 13, 2018 19:40

This was referenced Jun 14, 2018

BUG: decref in failure path; replace PyObject_Type by Py_TYPE #11336

Merged

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

Merged

mhvk closed this Jun 14, 2018

mhvk deleted the ufunc-override-call-class-only-once branch June 14, 2018 21:18

Uh oh!

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

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

Uh oh!

Conversation

mhvk commented Jun 13, 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!

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 Jun 13, 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 13, 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!

eric-wieser commented Jun 13, 2018

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 13, 2018

Uh oh!

mhvk commented Jun 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eric-wieser Jun 13, 2018 •

edited

Loading

eric-wieser Jun 13, 2018 •

edited

Loading

eric-wieser commented Jun 13, 2018 •

edited

Loading