Skip to content

BUG: prevent ufunc output to an already-broadcasted array #11667

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

mattip
Copy link
Member

@mattip mattip commented Aug 3, 2018

Fixes #2705.

I tried to find a balance between fixing all possible edge cases and keeping the code simple. The documentation for broadcast_arrays already warns against writing to these, this PR attempts to detect a pending, perhaps unintentional write in a func operation and raises a ValueError

@mattip mattip added 00 - Bug 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes component: numpy.ufunc labels Aug 3, 2018
@mattip mattip force-pushed the broadcast-0-strides branch from f015acb to f0be87f Compare August 3, 2018 00:47
@mattip mattip removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Aug 3, 2018
@mattip mattip force-pushed the broadcast-0-strides branch from f0be87f to 159c72e Compare August 3, 2018 00:56
@mattip
Copy link
Member Author

mattip commented Aug 3, 2018

It turns out not so trivial to find the equivalent of "writable operand with 0 stride and usable dim > 0."
I thought I had a good handle on this but

  • Reductions and accumulations can set an output stride = 0
  • Fancy indexing uses an iterator that never really iterates, it is used to calculate the strides.

@seberg
Copy link
Member

seberg commented Aug 4, 2018

Some of that we can handle, but my guess is its either more complicated to detect (or needs a flag), or just doesn't belong in nditer but instead in the ufunc machinery in this case. Not sure though. Other pain points could be e.g. ufunc.at (but that is basically indexing).

EDIT: Ah well, dunno, have to think a bit more to recall how this all worked....

@mhvk
Copy link
Contributor

mhvk commented Aug 5, 2018

This may indeed be better done in the ufunc setup - the fact that reduce uses this suggests that for the iterator one should continue to allow maximum freedom.

Also, a different and I think better solution is to just make broadcasted arrays read-only; this is indeed why as_strided has a writable flag added to it in numpy 1.12 and broadcast_to returns read-only arrays. The idea was to eventually do the same for broadcast_arrays, though it is not so easy to see how to do that...

@mattip mattip force-pushed the broadcast-0-strides branch from 6b1d3a1 to 52308ca Compare August 6, 2018 20:48
@mattip
Copy link
Member Author

mattip commented Aug 6, 2018

rebased to fix merge conflicts

@mattip mattip force-pushed the broadcast-0-strides branch from 52308ca to c1104ee Compare August 6, 2018 21:37
@mattip mattip force-pushed the broadcast-0-strides branch from c1104ee to 4249629 Compare August 6, 2018 23:27
@mattip
Copy link
Member Author

mattip commented Aug 8, 2018

I moved the check to the ufunc level, added the trivial test from the issue and modified memory_overlap tests to verify that if the error is triggered then ufunc(b, c, out=out) != ufunc(b, c). This should cover most of the common cases, without trying too hard to systematically catch them all.

@seberg
Copy link
Member

seberg commented Aug 10, 2018

I think I may have to think about it a while when I feel I have the time, but maybe Marten has a feel for it. First, I want to think a bit whether this approach is right (and if you were maybe right after all with the nditer approach). Second, I am a bit unsure whether a hard error is good.

@mhvk
Copy link
Contributor

mhvk commented Aug 10, 2018

I'm still not sure about this - I think I'd prefer to just not generate any writable broadcast arrays by default, and leaving any other usage up to the user, i.e., rather than prevent doing things that are potentially risky and fragile, make it such that it is a conscious choice to do so.

@mattip
Copy link
Member Author

mattip commented Dec 25, 2018

Closing. If any fix is needed it should be done in broadcast_arrays

@mattip mattip closed this Dec 25, 2018
@mattip mattip deleted the broadcast-0-strides branch March 4, 2019 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: wrong output of inplace operators x += y, x *= y, ... for nonstandard strides
3 participants