Skip to content

CLN: Remove MultiIndex._get_grouper_for_level #49690

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

Conversation

codamuse
Copy link
Contributor

@codamuse codamuse changed the title Cln/simplify get grouper for level CLN: Remove MultiIndex._get_grouper_for_level Nov 14, 2022
@codamuse
Copy link
Contributor Author

codamuse commented Nov 14, 2022

@rhshadrach Cleaner branch and passes tests locally (hopefully CI will work). Is there a reason though to even keep _get_grouper_for_level around or use it in the Grouping class? Around L483 in gouper.py I think I could just set self.grouping_vector = index_level.map(mapper) and achieve the same outcome. Nothing else uses _get_grouper_for_level() so is there a need to keep it around at all?

@jbrockmendel
Copy link
Member

@codamuse can you address the linting failures and merge main

@codamuse
Copy link
Contributor Author

@codamuse can you address the linting failures and merge main

@jbrockmendel updated. Do you think _get_grouper_for_level() makes sense to keep around? I don't think the name is even reflective of what that function does anymore (no levels to get a grouper and level isn't even an accepted argument). Since it's only used in grouper.py L483 would it be cleaner to simplify and change L483 to:

self.grouping_vector = index_level.map(mapper)

while dropping _get_grouper_for_level() or deprecating it some other way just in case?

None of the above reflected in this PR, just wondering why not do it that way.

@jbrockmendel
Copy link
Member

[...] would it be cleaner to simplify and change [...]

seems reasonable.

@jbrockmendel
Copy link
Member

LGTM pending green. Love to see more simplifications in grouper.py

@codamuse codamuse marked this pull request as ready for review November 30, 2022 12:23
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

lgtm

@rhshadrach rhshadrach added this to the 2.0 milestone Dec 1, 2022
@rhshadrach rhshadrach added Groupby Index Related to the Index class or subclasses labels Dec 1, 2022
@rhshadrach rhshadrach merged commit e35c0c4 into pandas-dev:main Dec 1, 2022
@rhshadrach
Copy link
Member

Thanks @codamuse - very nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Groupby Index Related to the Index class or subclasses MultiIndex
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLN: Remove MultiIndex._get_grouper_for_level
4 participants