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

Follow up PR: #28097 Simplify branch statement #29243

Merged
merged 1 commit into from
Jan 26, 2020
Merged

Follow up PR: #28097 Simplify branch statement #29243

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pandas/core/indexes/multi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,10 +1266,10 @@ def _get_grouper_for_level(self, mapper, level):
# Remove unobserved levels from level_index
level_index = level_index.take(uniques)

if len(level_index):
Copy link
Contributor

Choose a reason for hiding this comment

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

not what i meant

can u eliminate this branch 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.

@jreback

Ok. I eliminate branch. But I don't know how not to use branch and not conflict with test cases together. Could you give me a clue?

Copy link
Member

Choose a reason for hiding this comment

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

@jreback i think the blocker here is clarifying for this contributor what you're asking for

Copy link
Member

Choose a reason for hiding this comment

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

I don't think CI logs are around but do we know why this change fails CI? I think @jreback point is that this seems unnecessary so unclear why it is causing the failures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WillAyd
Because "Index.take"

            if allow_fill and fill_value is not None:
                msg = "Unable to fill values because {0} cannot contain NA"
                raise ValueError(msg.format(self.__class__.__name__))

cause fail.
Some Indexes use ".take" from "Index.take". If grouper = level_index.take(codes, fill_value=True) all the time and "level_index" use "take" from "Index.take", then raise exception.
So I changed check "level_index" empty or not. this check avoids from exception.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm OK. I guess that goes back to this line:

pandas/pandas/core/indexes/base.py

Line 804 in 2470690

f"Unable to fill values because {cls_name} cannot contain NA"

So I guess we would have to allow fill_value being passed to Index types that can't hold NA for this to work.

I think I'd be OK with that but any objections @jreback?

>>> pd.Index(range(5)).take([3], fill_value=1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pandas/core/indexes/base.py", line 901, in take
    raise ValueError(msg.format(self.__class__.__name__))
ValueError: Unable to fill values because RangeIndex cannot contain NA

>>> pd.Index(range(5)).astype(float).take([3], fill_value=1)
Float64Index([3.0], dtype='float64')

Copy link
Contributor Author

@proost proost Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@WillAyd
You are right. If Some level index in "MultiIndex", which can't hold NA do not pass flll_value. can exist. So there is need to check.

Copy link
Member

Choose a reason for hiding this comment

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

@proost I think you can replace the if check to be if level_index._can_hold_na: to at least be more explicit

grouper = level_index.take(codes)
else:
if level_index._can_hold_na:
grouper = level_index.take(codes, fill_value=True)
else:
grouper = level_index.take(codes)

return grouper, codes, level_index

Expand Down