-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
ENH: Deprecate writeable broadcast_array #12609
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
Conversation
Was going to answer on the list, but another (or two) emails of me are not being accepted by the mailing list right now: We have/used to have |
Looking at The |
The diagonal stuff is a bit cursed IIRC. @njsmith can probably give a quick hint on were it got stuck, I can't say I fully remember right now. I seem to recall that there was a lot of pushback or simply problems around the whole thing, I think it was one of the examples for "deprecations that did not turn out to go well" in the deprecations NEP or roadmap thingy. |
I can comment on diag and The original plan was to make both diag and multifield-indexes return views instead of (packed) copies. Part of the problem is that this breaks views of the result, because the views in both cases have padding bytes, but the copy does not. Now that the multifield stuff is in I guess we could either remove or repurpose |
The mailing list generated a few comments, with a request to not change the API as it understood that this may create overlapping memory. The Perhaps repurposing |
Belatedly: I'd be quite happy to just go directly to changing the default to |
6b14c91
to
7d28a6b
Compare
I have repurposed |
I guess the only question in my mind is whether it is right to make this flag a part of the python API, because that means we may need to keep supporting it forever. Eg, if someone writes a library which turns off the flag, that code needs to work even after we no longer use the flag anywhere. But, I think that is acceptable, and the flag seems useful to have around in case similar cases turn up in the future. I, for one, am fine merging without discussion on the list - this isn't going to break anyone's code. |
Oh, have to think about fully exposing it/this a bit more I guess. I somewhat thought we would re-purpose it for deprecation only (switching over to giving an error on such writes, unless someone opts in with |
OK, after discussing with Stephan Hoyer, I think we should morph this into:
I do remember that there was some push-back on the mailing list, but IIRC it was mostly one person and it was a bit unclear whether there was a real use-case around it. I do think we should not expose new public API for this, instead have a private function to set the flag (since we need it). EDIT: I can work on this if you prefer. |
7d28a6b
to
2e23b42
Compare
2e23b42
to
e7af42e
Compare
Needs #13463 to fix a bug with |
e7af42e
to
c78f8d3
Compare
Rebased. I forgot to add that fix to gh-13463, inserted it as a new commit here now, but maybe it would be better as its own PR. I hope you do not mind the forced pushing. |
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.
This looks good but I think the change log entry could be a bit clearer; hopefully my suggestions make sense. The rest is mostly nitpicks, though also one query.
If I recall, we converted it to a view and made it readonly with intent to change that later. At that point I'm forgetful, although ISTR that there was a reason I didn't precede to finish that program. |
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.
This now looks great. A few more wording comments for the changelog entry - hopefully me working out my confusing will end up helping others not be confused...
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.
OK, all great now!
Approach looks sounds, but some of the prose seems wrong, and there's an unhandled error path. |
I assume the flag gets propagated? a, b = np.broadcast_arrays(np.arange(3), np.arange(3)[:,None])
a.writeable # warns
a2 = a.view()
a2.writeable # but does this warn? |
Yes, the flag propagates. However, if after creating
|
Agreed that we don't need to fix that. Maybe worth documenting that behavior somehow, either just in the release notes or the warning message. Something about fixing up the flag before taking views such as by slicing. Putting the word "immediately" in the existing text might be enough. |
When the base is not an array (or generally when the flag of the base array was toggled), it is OK to allow setting the writeable flag to True, as long as any ancestor (especially the last one) is writeable.
831cf15
to
b31f896
Compare
@eric-wieser, @seberg: I rebased and squashed this. Any more review? |
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.
Didn't go through it super carefully, but I remember it being all fine. And tests seem thorough enough, some blank lines are missing in the release notes, but I can also do that later. Just in case someone still wants to comment, not merging it right now.
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
Sorry, and thanks for the additional ping. I will put this in. |
With cython code, this beatifully constructed PR sadly is giving false positives; see #13929 |
Relates to #2705, will fix it when deprecation warning is removed and readonly flag set. Replaces #11667.As suggested in #11667 and in the code, the view returned from
broadcast_arrays
should be madereadonly
.set theCurrently we leave theNPY_ARRAY_WARN_ON_WRITE
flag if the data is repeated across the array to fit the new shape.readonly
flagFalse
which can lead to errors.An alternative would be to immediately set thereadonly
flag False, with no deprecation cycle. Advantages to the alternative path:- we are disrupting workflows anyway, we could do it only once instead of twice (once to filter theDeprecationWarning
, once to remove the filter and handle thereadonly
change)- The current behavior is clearly wrong and will cause bugs if the returned arrays are written to.I will solicit responses from the mailing list
Edit: modified to set
NPY_ARRAY_WARN_ON_WRITE
Edit: modified to set a DeprecationWarning