-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
ENH: Refactor code to add is_view method for Series. #5853
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
This looks ok to me but it needs a docstring. |
Right. Done. |
This can go in 0.13.1, need to test for the exception being raised and add a release note, i'll merge. |
The exception code was already there, so I assume it works. Where should I add the release note? |
doc/source/[v0.13.1.txt,whatsnew.rst]. |
ENH: Refactor code to add is_view method for Series.
I'm confused. Isn't this opposed to the logic of the SettingWithCopy errors? Why isn't the resolution to make the BlockManager copy the array in place before doing the sort? (views share BlockManagers right?) |
Or maybe I'm not clear why this is a public method vs method on the SingleBlockManager. |
I'm confused too. What does providing an |
Okay, let me take a step back: what's the rationale for making this a public method? |
iIIRC the view stuff needs to be taken out sort/order/sort_index have an open issue to have their API fixed iirc |
Ok, I haven't kept up with those changes and considered this a harmless addition. |
Can we revert it temporarily and revisit after we push 0.13.1? |
Sure, but before that, can you explain the issue in a way I can grok? |
Issue that arose often was that people used chained assignment and accidentally set values on copies (accomplishing nothing). @jreback added a SettingWithCopy warning that checks for setting values on non-explicit copies (ie not from copy() or methods that return a new object). So now the general instruction is that you want to work with views not (accidental) copies. This method suggests the opposite mindset. It also exposes the user to a numpy implementation detail that (I'm speculating) doesn't make sense for pandas. |
you almost always get a new pandas object from operations (except when they r inplace) so an assignment could be working on a non explicit copy setting is_copy on cross sectional ops like take (which sort effectively does) normally will not matter/trigger except when a copy is for sure created (eg u know it's not a view) (the false positive is you are actually NOT doing a chained assignment) pandas differs from numpy here because we can make a guarantee that an op returns a copy |
I had a suspicion that part of the issue was an argument that this reifies the connection The chained assignment issue and fix I'm aware of (as well as some recoil by users on the warnings, I'm probably just missing 600 comments or so in other issues, too late for that. |
So you can check if a series is a view instead of waiting to get an error on sort or copying by default.