-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
Update MultiIndex checks #29494
Conversation
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.
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 |
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.
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
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.
because we're in cython and dont want to import non-cython
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. |
lgtm. merge master once again as I think we merged something related to this in last 4 hours. |
rebased+green |
thxs |
cc @WillAyd I'm holding off on most non-trivial groupby stuff, LMK if this is a problem.