CLN: Remove MultiIndex._get_grouper_for_level

[codamuse]

• closes CLN: Remove MultiIndex._get_grouper_for_level #49452 (Replace xxxx with the GitHub issue number)
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

[mroeschke]

I think according to #49452 (comment), we would like this to return a NotImplementedError

[rhshadrach]

I think according to #49452 (comment), we would like this to return a NotImplementedError

I think so - the idea being that for a MultiIndex, the current implementation of Index._get_grouper_for_level would likely give odd errors or incorrect results if called on a MultiIndex. We should fail clearly if this is attempted.

[codamuse]

Ok no problem. I'll push again today.

Sorry for some reason I thought I'd read a conversation where the decision was to remove it entirely instead of raise.

[jbrockmendel]

if its just a 1-liner, id suggest putting it in the base class method with a quick check

if self._is_multi:
    raise NotImplementedError(...)

that way you can add @final to the Index method, which I always find helpful

[codamuse]

if its just a 1-liner, id suggest putting it in the base class method with a quick check

if self._is_multi:
    raise NotImplementedError(...)

that way you can add @final to the Index method, which I always find helpful

Okay implemented as suggested in this last commit. But a few more comments/questions:

It seems to me that level is now only being asserted in _get_grouper_for_level but not used for anything else (and dropna not used at all). It looks like either grouper is set to the index or whatever mapping the mapper does on it and the only argument being used in here is mapper. If it's @final too then is there a reason to keep those extra arguments in? I'm sure there's a good reason and I'm just not familiar enough with any of the code base.

[rhshadrach]

If it's @final too then is there a reason to keep those extra arguments in? I'm sure there's a good reason and I'm just not familiar enough with any of the code base.

Prior to #49373 they were used for MultiIndex, but now they no longer are. These can be removed - and doing so here would be great.

[rhshadrach]

Looks like something went awry with pulling from main. In general, you should only need to do git pull upstream main in order to incorporate changes from main into your branch. At this point, there are a few options, but maybe the easiest would be to just siphon off your changes into a new branch. You can see changes from individual commits or consecutive groups of commits using the UI on github in the "Files changed" tab.

[codamuse]

Looks like something went awry with pulling from main. In general, you should only need to do git pull upstream main in order to incorporate changes from main into your branch. At this point, there are a few options, but maybe the easiest would be to just siphon off your changes into a new branch. You can see changes from individual commits or consecutive groups of commits using the UI on github in the "Files changed" tab.

Ya my bad, I rebased my branch. I'm going to just start a new branch as suggested.

While removing the args from the Index class it looks like _get_grouper_for_level is returning extra Nones as well. In a new clean branch I'll make all the changes I think can happen to clean up and you can tell me if I've gone too far? Unfortunately still haven't been able to run the tests locally. I'm using the docker container and panda-dev environment. Maybe a fresh pull etc and this will all be cleared up but to be sure, I should expect the tests to all pass locally right?

Thank you!

[rhshadrach]

I rebased my branch.

When I rebase, I first make sure all my changes are on origin (in case something goes wrong) and then do:

git merge-base HEAD upstream/main
[prints a commit hash]
git rebase -i [commit hash] -X theirs

Then pick the first commit and squash the rest. Sometimes if there are interleaving changes you get conflicts that need to be manually resolved, but most of the time everything goes through cleanly.

I'll make all the changes I think can happen to clean up and you can tell me if I've gone too far?

Sounds good!

I should expect the tests to all pass locally right?

For the most part, yes. Sometimes there are flakey tests, but otherwise, if the CI is green and your dev environment was created based on what's currently in main then the tests should pass.

[codamuse]

Will open a new one shortly