Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix "numpy scalar * array-like == performance horror"#3501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
charris merged 1 commit intonumpy:masterfrominducer:master
Jul 9, 2013

Conversation

@inducer
Copy link
Contributor

This pull requestfixes#3375. All tests that pass before this change also pass after. (There's a failure that seems unrelated.) The PR also adds a test that asserts that__array_priority__ gets looked at early enough to make sure no copy is attempted.

@seberg
Copy link
Member

Seems not quite that simple. The tests fail. It seems that this change as is breaks compatibility for array subclasses (i.e. matrix and masked arrays)? I don't have time to look into it, my best guess right now is that the reflected op, i.e.__radd__ also callsnp.add and then fails again, because whatever mechanism made sure that doesn't happen is somehow not active? Unfortunatly we have to be careful about what might be changed, there are probably no tests defining the current behaviour well.

@inducer
Copy link
ContributorAuthor

Ok, second attempt. This stays completely out of theufunc machinery and only applies to binary operators (as it should). Also see the lengthy comment in the code. Tests pass for me (I only rannumpy.test() the first time around)

@seberg
Copy link
Member

Can you explain to me why the array is converted currently? As far as I
understand the ufunc machinery expects that such objects as yours got
converted to an 0-d object array (or such). Where and why does this
happen? If we cannot fix it, can we hook into there to make your type
become a 0-d object array instead of a doing a full array conversion
process? Just thinking, but hooking your fix in further up doesn't
really sound right to me right now, though I will not look into it
deeply now.

@inducer
Copy link
ContributorAuthor

First, to answer your question:get_ufunc_arguments is called from these two sites:

That routine callsPyArray_FromAny on the ufunc arguments, which convertsMyThing into a 3x3 (IOW: large) object array of "scalar"MyThings. That's the core of the issue, because it is O(N) in the number of array elements. Nothing produces a 0-D array, and I'm not sure that would be correct or would fix anything.

Second, in my opinion, having__array_priority__ logic inumath is just broken. It should be ripped out. A tell-tale sign of this is that the logic is activated only for two-argument ufuncs with one output. What's so special about them? Why should they behave differently from three-argument ufuncs?

Another thing that's broken in the current code and would have to be fixed along with the issue above is that lots of places (such as1,2) usePyArray_Type->tp_as_number->nb_multiply to call themultiply ufunc (etc.). Because of__array_priority__, the semantics ofnb_multiply are no longer the same as the ufunc. Specifically,nb_multiply is required to to returnNotImplemented in cases of priority inversion, whereas the ufunc has a job to do and just needs to do it, without asking questions about priorities.

My patch above is a valid band aid for#3375. It does not claim to fix the issues I've mentioned. I do claim that it doesn't break anything that was previously working. Moreover, having priority logic "further up" (as you called it) is actually less risky, because itonly touches dispatch for binary operators. In other words,fewer code paths go through there than through the ufunc.

The issues I mentioned above should probably be filed as bugs of their own.

@seberg
Copy link
Member

Good points, it is true that it doesn't even make sense that ufuncs can possibly return NotImplemented, etc. I agree that this whole thing isn't ideal and likely it is the only reasonable thing to fix it where you fixed it. It is basically only because of how python handles subclassing with operators (i.e. Python knows that the subclass should be asked to handle the operator) that this code works at all. To be honest, I am not quite certain that array priority is necessary at all :).

I think the whole thing needs more thinking. I would digg a bit into checking other options before deciding that this perfect for myself, but I don't have the time right now. This creates one change I think, that I don't really mind too much, but should maybe be deprecated? If YourThing does not implement the__rop__ it would seem that things break?

@inducer
Copy link
ContributorAuthor

Array priority itself is a reasonable idea--it makes my example possible at all. Without it,numpy would not know that it's not supposed to try and convertMyThing into an array. (which is possible, but a terrible idea)

If some object declares an__array_priority__, but does not implement__rop__, an error will result as you point out. To me, that's fine. I would argue that that is correct behavior on the part ofnumpy, and a mistake on the part of the priority-declaring object, who claimed that it wanted to handle reverse arithmetic but then didn't. Onecould argue thatnumpy should check whether the corresponding__rop__ is implemented before returningNotImplemented, but no other part of Python works like that, and it inserts more expensive (and possibly brittle) checks into what is a performance-relevant code path. That's why I'm hesitant.

Copy link
Member

Choose a reason for hiding this comment

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

Blank first line in comment.

Copy link
Member

Choose a reason for hiding this comment

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

The comment is very large, I think you should omit the extended argument and just have a short explanation of what happens. The argument would look better as part of the pull request explanation, but it's late for that ;)

Copy link
Member

Choose a reason for hiding this comment

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

Basically just the first paragraph.

@charris
Copy link
Member

@seberg Are you happy with this?

@seberg
Copy link
Member

It seems like it is the right spot for such a check. I am still a little worried that breaking the case of people defining array priority but not supporting all the operators actually might affect code out there. I don't mind that as such, so maybe just try and see... What I am thinking of is mostly objects defining__array__ and for some reason also__array_priority__, i.e. objects in the general direction of PIL images or so? Though those shouldn't need array priority defined, so probably its fine.

Copy link
Member

Choose a reason for hiding this comment

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

float(17) isn't an array so I guess there's a mistake in the comment?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Nope, that's correct.float(17) * np.matrix(...) first ends up innp.matrix.__rmul__, which callsnp.dot, which callsPyArray_MatrixProduct2, which castsfloat(17) into an array scalar an then calls this place. :)

@inducer
Copy link
ContributorAuthor

Btw, I think I've addressed@charris's concerns in the commit I pushed earlier.

@charris
Copy link
Member

OK, one more thing. Because this changes behavior it should be mentioned indoc\release\1.8.0-notes.rst, probably under theChanges and maybe underCompatibility notes also if you think this might affect current code, as in combining__array_priority__ and__array__.

For next time note the standard prefixes for commit messages indoc/source/dev/gitwash/development_workflow.rst.

@inducer
Copy link
ContributorAuthor

Revised, with change comments and better commit message, as requested.

charris added a commit that referenced this pull requestJul 9, 2013
Fix "numpy scalar * array-like == performance horror"
@charrischarris merged commit6eeb612 intonumpy:masterJul 9, 2013
@inducer
Copy link
ContributorAuthor

Thanks for your help getting this merged!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

numpy scalar * array-like == performance horror

4 participants

@inducer@seberg@charris@njsmith

[8]ページ先頭

©2009-2025 Movatter.jp