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.

Sign up for GitHub

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

Jump to bottom

Deprecate DataFrame indexer for iloc setitem and getitem #39022

Merged
merged 14 commits into from
Mar 2, 2021
Merged

Deprecate DataFrame indexer for iloc setitem and getitem #39022

merged 14 commits into from
Mar 2, 2021

Conversation

phofl
Copy link
Member

@phofl phofl commented Jan 7, 2021 •
edited
Loading

@phofl phofl added Error Reporting Incorrect or improved errors from pandas Indexing Related to indexing on series/frames, not to indexes themselves labels Jan 7, 2021
jorisvandenbossche
jorisvandenbossche requested changes Jan 7, 2021
View reviewed changes
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.

See my note on the PR: #39004 (comment)

In addition, if we disallow this, I think it should be deprecated first for assignment (since that case was working right now)

@jreback
Copy link
Contributor

jreback commented Jan 8, 2021 •
edited
Loading

See my note on the PR: #39004 (comment)

In addition, if we disallow this, I think it should be deprecated first for assignment (since that case was working right now)

this is reasonable to deprecate for setitem and simply raise for getitem. @phofl if you can amend.

though I am tempted to simply call this a bug fix if setitem actually gives wrong behavior? (or is it actually somewhat correct?)

@phofl
Copy link
Member Author

phofl commented Jan 8, 2021

Don't know if I would call this wrong. It is counterintuitive for sure.

If you know that iloc does not align, you may expect the result, Additionally it only works if indexer has the same dimension as the object to index.

Will add a DeprecationWarning

@jreback
Copy link
Contributor

jreback commented Jan 8, 2021

Will add a DeprecationWarning

FutureWarning but thanks

@phofl
Copy link
Member Author

phofl commented Jan 8, 2021

Added the Warning

jreback
jreback reviewed Jan 8, 2021
View reviewed changes
pandas/core/indexing.py Outdated Show resolved Hide resolved
@phofl phofl changed the title Disallow DataFrame indexer for iloc setitem and getitem Deprecate DataFrame indexer for iloc setitem and getitem Jan 8, 2021
@phofl @jreback
Update pandas/core/indexing.py
82446b5 
Co-authored-by: Jeff Reback <jeff@reback.net>
@pep8speaks
Copy link

pep8speaks commented Jan 8, 2021 •
edited
Loading

Hello @phofl! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-02 19:53:34 UTC

jreback
jreback reviewed Jan 8, 2021
View reviewed changes
pandas/core/indexing.py Outdated Show resolved Hide resolved
phofl and others added 3 commits January 8, 2021 18:42
@phofl
Copy link
Member Author

phofl commented Jan 8, 2021

Added a deprecation whatsnew

@jreback jreback mentioned this pull request Jan 8, 2021
Closed
@jreback jreback added this to the 1.3 milestone Jan 8, 2021
jreback
jreback requested changes Jan 8, 2021
View reviewed changes
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 tiny comment

pandas/core/indexing.py Outdated
"DataFrame indexer for .iloc is deprecated and will be removed in"
"a future version.\n"
"consider using .loc with a DataFrame indexer for automatic alignment."
"a future version",
Copy link
Contributor

Choose a reason for hiding this comment

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

repeated 'a future version'

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx, changed

@phofl
Copy link
Member Author

phofl commented Jan 8, 2021

@jreback green

@jorisvandenbossche
Copy link
Member

Let's give this a bit more time, since the discussion in #39004 is still ongoing

@phofl
Merge branch 'master' of https://github.com/pandas-dev/pandas into 39004
a4a00f7 
� Conflicts:
�	doc/source/whatsnew/v1.3.0.rst
@phofl
Merge branch 'master' of https://github.com/pandas-dev/pandas into 39004
c9199d8 
� Conflicts:
�	doc/source/whatsnew/v1.3.0.rst
@jreback
Copy link
Contributor

jreback commented Jan 20, 2021

@phofl conflict and if you can rebase. i think this is fine to merge.

@phofl
Merge branch 'master' of https://github.com/pandas-dev/pandas into 39004
6ec9a9f 
� Conflicts:
�	doc/source/whatsnew/v1.3.0.rst
@phofl
Copy link
Member Author

phofl commented Jan 20, 2021

Done, failure unrelated

@jreback
Copy link
Contributor

jreback commented Jan 21, 2021

this looks fine to me. @jorisvandenbossche are you still objecting?

@jreback
Copy link
Contributor

jreback commented Feb 11, 2021

@phofl can you merge master and update

@phofl
Copy link
Member Author

phofl commented Feb 13, 2021

@jreback merged master, failure unrelated

@gooney47 gooney47 mentioned this pull request Feb 15, 2021
Closed
4 tasks
@jreback
Copy link
Contributor

jreback commented Feb 15, 2021 •
edited
Loading

everyone ok here ?

@jorisvandenbossche @phofl @jbrockmendel @gooney47

@jbrockmendel
Copy link
Member

LGTM

@phofl
Merge branch 'master' of https://github.com/pandas-dev/pandas into 39004
399dfc2 
� Conflicts:
�	pandas/tests/indexing/test_iloc.py
@phofl
Copy link
Member Author

phofl commented Mar 2, 2021

Are we good here?

jreback
jreback approved these changes Mar 2, 2021
View reviewed changes
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. ping on greenish

@phofl
Copy link
Member Author

phofl commented Mar 2, 2021

@jreback greenish

@jreback jreback merged commit dd2e721 into pandas-dev:master Mar 2, 2021
@jreback
Copy link
Contributor

jreback commented Mar 2, 2021

thanks @phofl

@phofl phofl deleted the 39004 branch March 4, 2021 21:59
@berland berland mentioned this pull request Apr 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche requested changes

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Error Reporting Incorrect or improved errors from pandas Indexing Related to indexing on series/frames, not to indexes themselves
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

BUG: IndexError: positional indexers are out-of-bounds iloc boolean indexing
5 participants
@phofl @jreback @pep8speaks @jorisvandenbossche @jbrockmendel