Skip to content

ENH: Allow where argument to override __array_ufunc__#23240

Merged
seberg merged 1 commit intonumpy:mainfrom
roytsmart:bugfix/ufunc_where_propagation
Mar 22, 2023
Merged

ENH: Allow where argument to override __array_ufunc__#23240
seberg merged 1 commit intonumpy:mainfrom
roytsmart:bugfix/ufunc_where_propagation

Conversation

@roytsmart
Copy link
Copy Markdown
Contributor

@roytsmart roytsmart commented Feb 17, 2023 •
edited
Loading

Currently, numpy.ndarray.__array_ufunc__ does not allow the where argument to override the default behavior of the ufunc.

This pull request allows this behavior by extending PyUFunc_CheckOverride to accept an additional argument that represents where, and checks to see if it overrides __array_ufunc__.

Closes Issue #23219

@roytsmart roytsmart force-pushed the bugfix/ufunc_where_propagation branch 7 times, most recently from 9b96e6d to 9dcb148 Compare February 18, 2023 19:32
@roytsmart
Copy link
Copy Markdown
Contributor Author

roytsmart commented Feb 18, 2023

Does anyone know why the Travis CI build is failing? I've tried making sure my branch is ahead of numpy:main, but it still seems to be timing out.

mhvk
mhvk approved these changes Feb 19, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Wow, I'm impressed with our original work and in particular @seberg's refactor that this is now so easy! It looks all good to me, with the travis failure surely a red herring. I'll approve but not merge yet, so @seberg can have a look too.

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Feb 19, 2023

Not sure why I always think of things after sending in a review, but what is still needed is a changelog fragment. And relevant updates to the documentation (and an update/clarification of the NEP??).

@seberg
Copy link
Copy Markdown
Member

seberg commented Feb 19, 2023

Agreed, a brief compat release note and modifying the NEP (I guess it is the main doc we have) would be good. Would also be nice to check up on what some __array_functions__ do that may be in a similar position (e.g. copyto maybe).

I was briefly considering if we should refactor out the append_if_new part of the loop into a function rather than the 3 way if, but probably not really clearer.

So I just agree with Marten, the code and test look nice and clean :).

@roytsmart roytsmart force-pushed the bugfix/ufunc_where_propagation branch 2 times, most recently from d43486a to c45be60 Compare February 19, 2023 19:38
mhvk
mhvk reviewed Feb 19, 2023
View reviewed changes
roytsmart
roytsmart commented Feb 19, 2023
View reviewed changes
@roytsmart roytsmart force-pushed the bugfix/ufunc_where_propagation branch 2 times, most recently from 90b0b5f to 983e2ae Compare February 19, 2023 20:46
mhvk
mhvk reviewed Feb 19, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

One nitpick and one suggestion, but really this looks great!

@charris charris changed the title Allow where argument to override __array_ufunc__ ENH: Allow where argument to override __array_ufunc__ Feb 19, 2023
@roytsmart roytsmart force-pushed the bugfix/ufunc_where_propagation branch 2 times, most recently from 93ea572 to 13f1e6e Compare February 19, 2023 22:46
seberg
seberg reviewed Feb 20, 2023
View reviewed changes
seberg
seberg reviewed Feb 20, 2023
View reviewed changes
@roytsmart roytsmart force-pushed the bugfix/ufunc_where_propagation branch 7 times, most recently from 2e6c5f6 to ae00248 Compare February 22, 2023 08:11
@seberg seberg mentioned this pull request Feb 22, 2023
Open
@roytsmart roytsmart force-pushed the bugfix/ufunc_where_propagation branch 3 times, most recently from 7f4915b to 5d9efe7 Compare February 23, 2023 02:38
mhvk
mhvk approved these changes Feb 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Looks really good! Only a suggestion for a textual change left. Re-approving, though probably @seberg should have a last look too.

@roytsmart roytsmart force-pushed the bugfix/ufunc_where_propagation branch 5 times, most recently from 86efeb4 to 9f7472c Compare February 24, 2023 08:01
@roytsmart roytsmart force-pushed the bugfix/ufunc_where_propagation branch from 9f7472c to f3f108d Compare February 24, 2023 08:22
roytsmart
roytsmart commented Feb 24, 2023
View reviewed changes
numpy/core/src/multiarray/methods.c
#include "strfuncs.h"
#include "array_assign.h"
#include "npy_dlpack.h"
#include "multiarraymodule.h"
Copy link
Copy Markdown
Contributor Author

@roytsmart roytsmart Feb 24, 2023

Choose a reason for hiding this comment

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

@seberg Is this the appropriate place to include this header?

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Feb 24, 2023

Not relevant for this PR, but "out" is gotten from the dictionary in the same way as "where" was here. @seberg - would it be worth raising a new issue with possible speed-ups? I might be able to do it in a bit, but am not sure I understood your other suggestion.

@seberg
Copy link
Copy Markdown
Member

seberg commented Feb 24, 2023

Not relevant for this PR, but "out" is gotten from the dictionary in the same way as "where" was here

Oh sorry, in that case there was no reason to update it really... This path is not hot anyway (I suspect few users use super().__array_function__).

No other suggestion beyond using interned strings. array-function passes types, which means you can decide whether to defer without ever looking at the exact arguments. I doubt we want to go into looking into an iteration of how dispatching works here though.

@roytsmart
Copy link
Copy Markdown
Contributor Author

roytsmart commented Feb 24, 2023

@seberg, if we wanted to defer using types, wouldn't we have to change the signature of __array_ufunc__?

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Feb 24, 2023

@byrdie - indeed, which would be hard to do... Might need some opt-in signal or so.

@roytsmart
Copy link
Copy Markdown
Contributor Author

roytsmart commented Feb 24, 2023

Yeah, I think that's probably out of scope for this PR, though I agree it would probably be better/faster if we did do that!

@roytsmart
Copy link
Copy Markdown
Contributor Author

roytsmart commented Mar 2, 2023

@seberg, what can I do to get this merged?

@seberg
Copy link
Copy Markdown
Member

seberg commented Mar 22, 2023

Considering that nobody voiced concerns, I think it is time to give this a shot. If anyone has concerns we could pull it out (at least easily before the next final release).

Thanks @byrdie.

@seberg seberg merged commit b35aac2 into numpy:main Mar 22, 2023
@roytsmart
Copy link
Copy Markdown
Contributor Author

roytsmart commented Mar 22, 2023

Thanks @seberg and @mhvk!

@roytsmart roytsmart deleted the bugfix/ufunc_where_propagation branch March 22, 2023 16:09
@mhvk mhvk mentioned this pull request Mar 23, 2023
Closed
@dcherian dcherian mentioned this pull request Jul 7, 2023
Open
4 tasks
@timmysilv timmysilv mentioned this pull request Oct 25, 2023
Merged
@ssche ssche mentioned this pull request Jan 2, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg left review comments

@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

01 - Enhancement

Projects

NumPy first-time contributor PRs
Status: Completed

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@roytsmart @mhvk @seberg @charris