Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork11.9k
BUG: Fix calling ufuncs with a positional output argument.#10479
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…lt to inherit the output's maskThis brings `np.add(a, b, out)` in line with `np.add(a, b, out=out)`.These previously differed becausenumpygh-10459 causes them to call __array_wrap__ in different ways (with and without the output argument in the context tuple, respectively).Since the data in the `out` argument is never used by ufuncs, it seems consistent that the mask should not be either.
mhvk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The changes are OK, though see my comment. I'd argue to remove the stanza that never can get called, since I don't see how it can be correct. But also fine to do it in follow-up, since I guess it is more MAINT than BUG
| ifdomainisnotNone: | ||
| # Take the domain, and make sure it's a ndarray | ||
| iflen(args)>2: | ||
| iflen(input_args)>2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not directly related, but this is very strange -- there are no ufuncs with more than two input arguments, so this path will never get taken... And before this would only be taken if there was an output argument, for which the domain would then be checked. Which makes no sense at all!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there are no ufuncs with more than two input arguments, so this path will never get taken
There's nothing to stop a third party (or scipy?) adding one like this, so it's nice to try to support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then they also have to update the domain dict -- not impossible, I guess, but why are they then forced to calculate the domain by reduction rather than just by getting all the arguments? This is definitely not what I would expect... for most operations, the masking depends very differently on each argument (think division...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, this doesn't make sense as is - expanding the tuple arguments is the right thing to do every time. I can fix this much later today, either in this PR or a separate one.
Doing it in a separate one means that I can also rebase the__array_prepare__ PR on top of this tonight.
mhvk commentedJan 26, 2018
The circleci error seems unrelated -- the whole installation failed. |
mhvk commentedJan 26, 2018
OK, let's do the fix to So, I'll merge this one now. |
Uh oh!
There was an error while loading.Please reload this page.
Currently, this causes the result to inherit the output's mask.
This brings
np.add(a, b, out)in line withnp.add(a, b, out=out).These previously differed becausegh-10459 causes them to callarray_wrap in different ways (with and without the output argument in the context tuple, respectively).
Since the data in the
outargument is never used by ufuncs, it seems consistent that the mask should not be either.