Skip to content

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

Conversation

rekcahpassyla
Copy link
Contributor

Proposed fix for #10193

@jreback
Copy link
Contributor

jreback commented May 22, 2015

see my comments on the actual issue. This needs to test that it is:

  • hitting the SetttingWIthCopy path
  • affecting the referenced Series (e.g. you have a view in this case).

@rekcahpassyla
Copy link
Contributor Author

I had another go- I looked at how DataFrame handled the setting of is_copy and then raising SettingWithCopyError or SettingWithCopyWarning, and tried to replicate that logic in Series.

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 chained_assignment option to None and back again.

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.

@rekcahpassyla
Copy link
Contributor Author

Noted the build failures. Investigating.

@rekcahpassyla rekcahpassyla force-pushed the empty_series_with_freq_setitem branch 3 times, most recently from 7bf263b to 1dffd7b Compare May 27, 2015 10:45
@jreback jreback added Bug Indexing Related to indexing on series/frames, not to indexes themselves API Design labels May 27, 2015
@jreback jreback added this to the 0.17.0 milestone May 27, 2015
@@ -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()
Copy link
Contributor

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

@rekcahpassyla rekcahpassyla force-pushed the empty_series_with_freq_setitem branch from 1dffd7b to ac3b656 Compare May 28, 2015 08:40
@rekcahpassyla
Copy link
Contributor Author

Requested changes from review have been made, also replaced other existing instances of pd.set_option('chained_assignment', ...) with context manager, to be consistent.

@jreback
Copy link
Contributor

jreback commented Jun 2, 2015

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?

@shoyer
Copy link
Member

shoyer commented Jun 3, 2015

@jreback can you clarify the specific behavior you're not sure about?

@jreback
Copy link
Contributor

jreback commented Jun 3, 2015

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.

In [1]: s = Series([1,2,3])

In [2]: s
Out[2]: 
0    1
1    2
2    3
dtype: int64

In [3]: s2 = s[1:]

In [4]: s2
Out[4]: 
1    2
2    3
dtype: int64

In [5]: s2.iloc[0] = 5

In [6]: s2
Out[6]: 
1    5
2    3
dtype: int64

In [7]: s
Out[7]: 
0    1
1    5
2    3
dtype: int64

@shoyer
Copy link
Member

shoyer commented Jun 3, 2015

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:

IMO warnings are almost never a good way to communicate usage notes to users for exactly this reason. They annoy people who know what they are doing, and they rarely provide enough context for those who don't.

@rekcahpassyla
Copy link
Contributor Author

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?

@jreback
Copy link
Contributor

jreback commented Jun 18, 2015

closing in favor of #10379

@jreback jreback closed this Jun 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design Bug Indexing Related to indexing on series/frames, not to indexes themselves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants