Skip to content

Fix MultiIndex .loc "Too Many Indexers" with None as return value #34450

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
Merged
merged 8 commits into from
Jun 3, 2020
Merged

Fix MultiIndex .loc "Too Many Indexers" with None as return value #34450

merged 8 commits into from
Jun 3, 2020

Conversation

pedrooa
Copy link
Contributor

@pedrooa pedrooa commented May 29, 2020

Three changes in indexing.py :

  • function _handle_lowerdim_multi_index_axis0 default return value was None, now it raises an Exception.
  • function _getitem_lowerdim checks for a raised exception instead of a value None.
  • function _getitem_nested_tuple checks for a raised exception instead of a value None(had to be changed because it also uses _handle_lowerdim_multi_index_axis0)

@pedrooa pedrooa marked this pull request as draft May 29, 2020 02:42
WillAyd
WillAyd reviewed May 29, 2020
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. We will want to be more specific than just raising Exception as that is too generic to be useful, but let's see if we can refactor a bit

pandas/core/indexing.py Outdated
@@ -1065,7 +1069,7 @@ def _handle_lowerdim_multi_index_axis0(self, tup: Tuple):
if len(tup) <= self.obj.index.nlevels and len(tup) > self.ndim:
raise ek

return None
raise Exception("No label returned")
Copy link
Member

Choose a reason for hiding this comment

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

Does this not already raise a KeyError a few lines up?

@WillAyd WillAyd changed the title BUG: Fixes Issue #34318 Fix MultiIndex .loc "Too Many Indexers" May 29, 2020
@WillAyd WillAyd changed the title Fix MultiIndex .loc "Too Many Indexers" Fix MultiIndex .loc "Too Many Indexers" with None as return value May 29, 2020
@pedrooa pedrooa marked this pull request as ready for review June 2, 2020 01:34
jreback
jreback requested changes Jun 2, 2020
View reviewed changes
pandas/tests/series/indexing/test_indexing.py Outdated
# GH34318: test that you can access a None value using .loc through a Multiindex

expected = None
result = pd.Series([None], pd.MultiIndex.from_arrays([["Level1"], ["Level2"]])).loc[
Copy link
Contributor

Choose a reason for hiding this comment

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

can you write this in a more readable way

s = Series(......)
result = s.loc....
assert result is None

Copy link
Contributor

Choose a reason for hiding this comment

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

pls add all the examples here as well: #34318 (comment)

@jreback
Copy link
Contributor

jreback commented Jun 2, 2020

pls also add a whatsnew note, indexing bug fixes for 1.1

@jreback jreback added the Indexing Related to indexing on series/frames, not to indexes themselves label Jun 2, 2020
jreback
jreback requested changes Jun 2, 2020
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 green after tests move.

pandas/tests/series/indexing/test_indexing.py Outdated
@@ -853,6 +853,23 @@ def test_setitem_slice_into_readonly_backing_data():
assert not array.any()


def test_access_none_value_in_multiindex():
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make a new file in this dir: test_multiindex.py

we have the MI tests elsewhere so ok starting a new file here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I copy all the imports from test_indexing.py?

Copy link
Contributor

Choose a reason for hiding this comment

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

you need whatever imports make flake happy :->

@jreback jreback added this to the 1.1 milestone Jun 2, 2020
jreback
jreback reviewed Jun 2, 2020
View reviewed changes
pandas/tests/series/indexing/test_multiindex.py Outdated
from pandas import (
Categorical,
DataFrame,
IndexSlice,
Copy link
Contributor

Choose a reason for hiding this comment

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

you are going to fail linting here. see https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#python-pep8-black

for how to install pre-commit hooks to avoid this issue

@jreback jreback merged commit 48e03f3 into pandas-dev:master Jun 3, 2020
@jreback
Copy link
Contributor

jreback commented Jun 3, 2020

thanks @pedrooa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Indexing Related to indexing on series/frames, not to indexes themselves MultiIndex
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

BUG: "IndexingError: Too many indexers" when accessing a None value using .loc through a MultiIndex
3 participants
@pedrooa @jreback @WillAyd