DEP: Warn MaskedArray will return views of mask when sliced #7020
Conversation
|
Please give me your feedback. If you think I need to change the wording, please let me know how you would like to see it changed. If the warning raised needs to be changed, please let me know. Also, if there are other places that should have a warning please let me know where to add them. Thanks in advance. |
b8283dd to
3b0cc0c
Compare
|
It looks like AppVeyor has hung. Could someone please restart it? Travis is passing. |
|
AppVeyor is just really slow. It is about 25 minutes at best and we have low priority, so it can be much worse. |
|
Yeah, I was just worried as it was stuck at the same line in the first build for several minutes. It seems to be moving now. I haven't worked with AppVeyor much. Does low priority mean that they might suspend our builds while someone else's runs? |
|
I'm not sure how they do it, but as a 'free' as in beer user we don't get no respect ;) |
numpy/ma/core.py
Outdated
There was a problem hiding this comment.
Might add a note before the warning, here and below, with the date and release of the warning. That will help us keep track of how much time has passed since. Something like
# 01/15/2016 1.11.0
would do.
There was a problem hiding this comment.
Sure, I added some comments before the warnings.
|
We should probably at least think at some point about whether it's worth bumping up our appveyor capacity. It's $50/month/parallel build, so $100/month to ~triple our current speeds. Not a huge amount of money in the grand scheme of things. |
|
Not that I feel like I have any say in financial decisions here, it is probably worth considering from a productivity viewpoint as we have run into one case within the last week where faster processing of the AppVeyor build queue would have caught and helped fix a bug unique to Windows. ( #6991 ) |
|
I seem to remember that another project I'm subscribed to contacted Appveyor and asked for a beefier free account, and got it. Maybe worth trying? |
|
It's generally a good idea to keep the small fixup commits squashed with the originals |
|
Doh, I messed up the numbers above -- those are the normal rate, but then there's a 50% discount for F/OSS projects, so it'd actually just be $50/month to enable 3 parallel builds. |
|
@charris, definitely, I plan to squash once we are getting closer to ready. I just didn't want to block up the build queues while you guys are working on last minute changes for 1.11.0. |
|
New commits also restart the build, so six of one, half dozen of the other. |
|
True, wasn't thinking about the merge commits created by PRs in testing. |
14068d3 to
1184e72
Compare
|
@charris BTW, I did squash them. |
… behavior of `MaskedArray`'s masks is changing.
… of their masks when they are also returning views of their data.
1184e72 to
4989360
Compare
|
Good ;) I'll be out for a while as the checks complete, hopefully this can go in when I get back. |
|
Sounds good. Travis is done. Looks like AppVeyor has just started on this one though. |
There was a problem hiding this comment.
Minor comment: Do we need warnings in __getitem__? The proposed change shouldn't have any effect on someone who never modifies the mask, right? It's only in __setitem__ that code might break.
Also, perhaps we can be even fancier and only show the warning if we know self.data is a view: In __setitem__ do if not self.data.owndata: warnings.warn('...').
There was a problem hiding this comment.
Do we need warnings in
__getitem__?
The return value is changing. Personally, I think that merits a warning.
The proposed change shouldn't have any effect on someone who never modifies the mask, right?
Could the change not effect them? Maybe, but I'd rather they start thinking about it now instead of leaving them in the dark until the breaking release; then have them discover some behavior is broken.
It's only in
__setitem__that code might break.
That's supposing they are following the rules and not using some hack. We should still warn them even when they are doing the wrong thing. Hence why I like the __getitem__ warning. Though I thought really hard about adding a warning in __new__ as well, but decided that might be too much.
Also, perhaps we can be even fancier and only show the warning if we know
self.datais a view...
I suppose, but I think we have just as much of a chance getting this wrong by being too tricky. Namely, not emitting a warning when we should. The current behavior is straightforward. If the user calls __setitem__, they are always warned about the breaking change that is pending.
|
All CIs check out. |
There was a problem hiding this comment.
Don't need the +, although it doesn't hurt.
There was a problem hiding this comment.
And this will be deleted in the future.
There was a problem hiding this comment.
Sure, would you rather the +s be removed?
…copy DEP: Warn MaskedArray will return views of mask when sliced
|
Thanks @jakirkham . Yeah, we thank everyone. |
|
Nothing wrong with saying thanks. ;) Thanks for getting this one in there @charris. Thanks everyone for your feedback. If you find you still don't like something, I am happy to make changes, as needed. |
|
And thank you @jakirkham for the PR! Your answer above made sense. |
|
Glad it made sense. If you decide something about it needs to be changed in the future @ahaldane, feel free to let me know and we can adjust it as needed. |
Fixes #7012
Related: #5580
This deprecates the copying of masks when slicing
MaskedArrays in favor of providing views when a view of the data is provided.Notes the changing behavior in the 1.11.0 release notes.
Adds
FutureWarnings in these places:__getitem____setitem__