Skip to content
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/DEPR: loc.__setitem__ incorrectly accepting positional slices #31840

Merged
merged 24 commits into from Mar 8, 2020

Conversation

jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Feb 10, 2020

I plan to flesh out the tests before this goes in.

cc @toobaz

@jorisvandenbossche
Copy link
Member

Since this has been such a long time behavior (also used in our own tests ..), I think we should maybe consider raising a warning first.

@jbrockmendel
Copy link
Member Author

I think we should maybe consider raising a warning first.

Yah, i'm leaning towards a deprecation cycle, too.

@jbrockmendel jbrockmendel added Bug Indexing Related to indexing on series/frames, not to indexes themselves labels Feb 19, 2020
@jreback jreback added this to the 1.1 milestone Feb 22, 2020
@jreback
Copy link
Contributor

jreback commented Feb 22, 2020

can you rebase, and agree with @jorisvandenbossche this is worth deprecating (even though its an incorrect behavior)

@jbrockmendel
Copy link
Member Author

updated per comments, but i think the "Do X instead" part isnt quite right. "Use iloc instead" works for Series, but doesnt work as well for DataFrame

@jreback
Copy link
Contributor

jreback commented Feb 23, 2020

lgtm. can you add to the deprcations issue & rebase; ping on green.

@jbrockmendel
Copy link
Member Author

added to the deprecation tracker, rebased, and green (though the CI is broken for unrelated reasons)

However I would still like to get a better message for users suggesting an alternative to use for the DataFrame case. cc @jorisvandenbossche for suggestions?

@jbrockmendel
Copy link
Member Author

@WillAyd suggestions for a more helpful message for the 2D case?

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2D case is something like this?

df.iloc[:4, ['A', 'B'] = 1

I'd say something like "use .loc with labels, or .iloc with positions instead." I don't think we can really say which will be easier for the user in the error message, so just don't take a position.

pandas/core/indexes/base.py Outdated Show resolved Hide resolved
@jorisvandenbossche
Copy link
Member

I'd say something like "use .loc with labels, or .iloc with positions instead." I don't think we can really say which will be easier for the user in the error message, so just don't take a position.

+1 on that. Trying to explain both options more in detail will get a bit complex for the deprecation warning I think

@jbrockmendel
Copy link
Member Author

I'd say something like "use .loc with labels, or .iloc with positions instead." I don't think we can really say which will be easier for the user in the error message, so just don't take a position.

Makes sense, updated.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. unless @jorisvandenbossche or @TomAugspurger have other comments, merge away

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one comment about the stacklevel, for the rest looks good

with pytest.raises(TypeError, match=msg):
df.loc[1:3, 1]

with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would optimise the stacklevel so it works correctly for dataframe, instead of series.
I would think doing this with dataframe will be more common (it's also the case that is more difficult to solve). At least in our tests it was mainly with dataframes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated stacklevel

@jorisvandenbossche jorisvandenbossche changed the title BUG: loc.__setitem__ incorrectly accepting positional slices BUG/DEPR: loc.__setitem__ incorrectly accepting positional slices Mar 3, 2020
@jreback jreback merged commit 957fc3c into pandas-dev:master Mar 8, 2020
@jreback
Copy link
Contributor

jreback commented Mar 8, 2020

thanks @jbrockmendel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Indexing Related to indexing on series/frames, not to indexes themselves
Projects
None yet
4 participants