-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Check for size != 0 before trying to insert #10193 #10194
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
BUG: Check for size != 0 before trying to insert #10193 #10194
Conversation
see my comments on the actual issue. This needs to test that it is:
|
I had another go- I looked at how Added tests for the requisite error / warning being raised, and that the original series is unchanged. Had to change a couple of other tests to change the I haven't squashed commits as it's still WIP. Will look again to see how I can make it more consistent with existing functionality, but here's something for now. |
Noted the build failures. Investigating. |
7bf263b
to
1dffd7b
Compare
@@ -1439,6 +1441,30 @@ def test_setitem(self): | |||
expected = self.series.append(Series([1],index=['foobar'])) | |||
assert_series_equal(s,expected) | |||
|
|||
# Test for issue #10193 | |||
series = pd.TimeSeries() |
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.
don't use TimeSeries
, just Series
to avoid SettingWithCopyError where not desired
1dffd7b
to
ac3b656
Compare
Requested changes from review have been made, also replaced other existing instances of |
I have to think about this again. This is a 'feature' of numpy. In that if you set something on a view of something else it works, and changes the original, as that is the point. However, in pandas, objects are pure and changing one shouldn't impact another one, so in effect, even though its supported, the view machinery is effectively frowned upon. @shoyer @jorisvandenbossche thoughts? |
@jreback can you clarify the specific behavior you're not sure about? |
Well its just general view semantics for Series. These almost always apply because these are a single dtyped, while for frames depends on whether its multi-dtype and what you are setting. I am just thinking that this might be confusing.
|
I agree, views are a bit confusing, but they're also a fundamental part of the NumPy ecosystem -- and they make a lot of efficient things possible. I would definitely prefer not to add more SettingWithCopy warnings. To quote @mwaskom from Twitter:
|
For my own edification; is the issue here that setting a value on a Series which is a view should always pass, because "in pandas, objects are pure and changing one shouldn't impact another one", or that it should always fail? |
closing in favor of #10379 |
Proposed fix for #10193