-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
Bug in loc did not raise KeyError when missing combination with slice(None) was given #37764
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
pandas/core/indexing.py
Outdated
| if is_nested_tuple(key, labels): | ||
| locs = labels.get_locs(key) | ||
| if not len(locs): | ||
| raise KeyError(key) |
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.
if this is going to raise (not 100% sure, but leaning towards yes), i think it should happen in MultiIndex.get_locs
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.
Could not find any information in the docs, if get_locs should raise when the combination of both keys is not found in MultiIndex. Would prefer that too, but did not want to break anything, hence my question above. Will try to introduce this there
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.
We are raising in get_locs now. This should be much more stable
|
... Edit: Did not think this through. |
jreback
left a comment
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 ping on green.
| ) | ||
| df = DataFrame(np.random.rand(4, 3), index=mi) | ||
| msg = r"\('b', '1', slice\(None, None, None\)\)" | ||
| with pytest.raises(KeyError, match=msg): |
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.
can you also add the case where the 3rd level is level out (should also raise)
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.
Done
� Conflicts: � doc/source/whatsnew/v1.2.0.rst
|
lgtm @jbrockmendel if any comments. |
|
LGTM Thanks for tackling all these indexing issues @phofl, they can be a real doozy. FWIW we've done a lot of semi-recent cleanup of the indexing code but MultiIndex has received relatively little attention. Probably some relatively low-hanging fruit there. |
|
Thanks, I will have a look for the MultiIndex code paths |
|
@phofl can you merge master, the CI should be fixed |
� Conflicts: � doc/source/whatsnew/v1.2.0.rst
� Conflicts: � doc/source/whatsnew/v1.2.0.rst
|
Merged master |
|
thanks @phofl |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffAdditional question: When an non existent key is given for
get_locsit raises aKeyError. When just a missing combination of keys is given ((b,1)in the new test), while the individual keys exist, an empty array is returned. Is this the expected behavior?