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: Raise KeyError when indexing a Series with MultiIndex #26155

Merged
merged 17 commits into from Apr 26, 2019

Conversation

ryanreh99
Copy link
Contributor

@ryanreh99 ryanreh99 commented Apr 20, 2019

@pep8speaks
Copy link

pep8speaks commented Apr 20, 2019

Hello @ryanreh99! 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 2019-04-24 14:32:03 UTC

@codecov
Copy link

codecov bot commented Apr 20, 2019

Codecov Report

Merging #26155 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26155      +/-   ##
==========================================
- Coverage      92%   91.98%   -0.02%     
==========================================
  Files         175      175              
  Lines       52371    52378       +7     
==========================================
- Hits        48184    48182       -2     
- Misses       4187     4196       +9
Flag Coverage Δ
#multiple 90.54% <100%> (-0.01%) ⬇️
#single 40.74% <0%> (-0.15%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexing.py 90.56% <100%> (-0.32%) ⬇️
pandas/io/gbq.py 75% <0%> (-12.5%) ⬇️
pandas/core/frame.py 96.9% <0%> (-0.12%) ⬇️
pandas/core/indexes/base.py 96.94% <0%> (-0.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c79b7bb...31df19b. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 20, 2019

Codecov Report

Merging #26155 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26155      +/-   ##
==========================================
- Coverage   91.98%   91.97%   -0.02%     
==========================================
  Files         175      175              
  Lines       52371    52375       +4     
==========================================
- Hits        48175    48171       -4     
- Misses       4196     4204       +8
Flag Coverage Δ
#multiple 90.52% <100%> (-0.01%) ⬇️
#single 40.7% <0%> (-0.16%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexing.py 90.53% <100%> (-0.35%) ⬇️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/computation/engines.py 88.33% <0%> (-0.2%) ⬇️
pandas/core/frame.py 96.9% <0%> (-0.12%) ⬇️
pandas/core/indexes/base.py 96.94% <0%> (-0.06%) ⬇️
pandas/core/algorithms.py 94.8% <0%> (+0.01%) ⬆️
pandas/util/testing.py 90.71% <0%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 947bd76...2309e14. Read the comment docs.

@gfyoung gfyoung added the Error Reporting Incorrect or improved errors from pandas label Apr 20, 2019
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.

you additions make this way more complicated, see if you can simplify

pandas/core/indexing.py Outdated Show resolved Hide resolved
pandas/core/indexing.py Outdated Show resolved Hide resolved
pandas/core/indexing.py Outdated Show resolved Hide resolved
pandas/core/indexing.py Show resolved Hide resolved
pandas/core/indexes/base.py Outdated Show resolved Hide resolved
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.

pls add a whatsnew note in bug fixes; indexing section. the code change looks good.

pandas/tests/indexing/multiindex/test_loc.py Show resolved Hide resolved
@jreback jreback added this to the 0.25.0 milestone Apr 21, 2019
pandas/core/indexing.py Outdated Show resolved Hide resolved
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.

small comments, ping on green.

pandas/core/indexing.py Outdated Show resolved Hide resolved
pandas/tests/indexing/multiindex/test_loc.py Show resolved Hide resolved
@ryanreh99
Copy link
Contributor Author

hello @jreback ping

@@ -327,6 +327,7 @@ Indexing
^^^^^^^^

- Improved exception message when calling :meth:`DataFrame.iloc` with a list of non-numeric objects (:issue:`25753`).
- Bug in :meth:`DataFrame.loc` and :meth:`Series.loc` where KeyError was not raised for a ``MultiIndex``. Now, IndexingError takes precedence over KeyError (:issue:`14885`).
Copy link
Contributor

Choose a reason for hiding this comment

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

use double backticks on KeyError; use :class:`MultiIndex` ; maybe something more like

Bug in :meth:DataFrame.loc and :meth:Series.loc where KeyError was not raised for a MultiIndex when the key was longer that the number of levels in the :class:MUutiIndex. (:issue:14885).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback actually when key is longer than the levels of the multiindex IndexingError: Too many indexers was raised and is still raised. So would this be more proper?

Bug in :meth:`DataFrame.loc` and :meth:`Series.loc` where ``KeyError`` was not raised for a ``MultiIndex`` when the key was less than or equal to the number of levels in the :class:`MultiIndex` (:issue:`14885`).

@ryanreh99
Copy link
Contributor Author

ryanreh99 commented Apr 24, 2019

made a change to the if statement as a KeyError wasn't raised for a particular condition. Added that test.

@jreback jreback merged commit e464a88 into pandas-dev:master Apr 26, 2019
@jreback
Copy link
Contributor

jreback commented Apr 26, 2019

thanks @ryanreh99 very nice

@ryanreh99
Copy link
Contributor Author

@jreback thanks for all the help :D

@ryanreh99 ryanreh99 deleted the my-first-branch branch April 26, 2019 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong "Too many indexers" error message when indexing a Series with MultiIndex
4 participants