Skip to content

Update MultiIndex checks #29494

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

Merged
merged 2 commits into from
Nov 13, 2019
Merged

Update MultiIndex checks #29494

merged 2 commits into from
Nov 13, 2019

Conversation

jbrockmendel
Copy link
Member

cc @WillAyd I'm holding off on most non-trivial groupby stuff, LMK if this is a problem.

@jbrockmendel
Copy link
Member Author

@jreback @WillAyd gentle ping for this and #29456. Neither is important on its own, but both will have merge conflicts with more-important PRs on their way.

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.

Minor comment but not a big deal. OK with merging as is

@@ -644,7 +644,7 @@ def compute_reduction(arr, f, axis=0, dummy=None, labels=None):

if labels is not None:
# Caller is responsible for ensuring we don't have MultiIndex
assert not labels._has_complex_internals
assert labels.nlevels == 1
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to do an isinstance check here? Not a strong blocker usage is mixed; might be nice to settle on one way of doing this

Copy link
Member Author

Choose a reason for hiding this comment

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

because we're in cython and dont want to import non-cython

@WillAyd WillAyd added this to the 1.0 milestone Nov 9, 2019
@jreback
Copy link
Contributor

jreback commented Nov 12, 2019

can we completely remove _has_complex_internals? also IIRC there is an open PR about removing it (should close).

@jbrockmendel
Copy link
Member Author

can we completely remove _has_complex_internals? also IIRC there is an open PR about removing it (should close).

I pushed a rebase on to that this morning. After this I'll probably have to rebase it again and thatll remove _has_complex_internals entirely.

@jreback
Copy link
Contributor

jreback commented Nov 12, 2019

lgtm. merge master once again as I think we merged something related to this in last 4 hours.

@jbrockmendel
Copy link
Member Author

rebased+green

@jreback jreback merged commit 0e1d56a into pandas-dev:master Nov 13, 2019
@jreback
Copy link
Contributor

jreback commented Nov 13, 2019

thxs

@jbrockmendel jbrockmendel deleted the gb-mi branch November 13, 2019 03:52
Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants