-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG/DEPR: loc.__setitem__ incorrectly accepting positional slices #31840
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
* REF: Move Loc-only methods to Loc * rebase fixup * merge fixup
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. |
Yah, i'm leaning towards a deprecation cycle, too. |
can you rebase, and agree with @jorisvandenbossche this is worth deprecating (even though its an incorrect behavior) |
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 |
lgtm. can you add to the deprcations issue & rebase; ping on green. |
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? |
@WillAyd suggestions for a more helpful message for the 2D case? |
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.
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.
+1 on that. Trying to explain both options more in detail will get a bit complex for the deprecation warning I think |
Co-Authored-By: Tom Augspurger <[email protected]>
Makes sense, updated. |
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.
lgtm. unless @jorisvandenbossche or @TomAugspurger have other comments, merge away
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.
Only one comment about the stacklevel, for the rest looks good
pandas/tests/indexing/test_loc.py
Outdated
with pytest.raises(TypeError, match=msg): | ||
df.loc[1:3, 1] | ||
|
||
with tm.assert_produces_warning(FutureWarning, check_stacklevel=False): |
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.
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
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.
updated stacklevel
thanks @jbrockmendel |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
I plan to flesh out the tests before this goes in.
cc @toobaz