Skip to content

CLN: Remove MultiIndex._get_grouper_for_level #49452

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

Closed
rhshadrach opened this issue Nov 1, 2022 · 14 comments · Fixed by #49690
Closed

CLN: Remove MultiIndex._get_grouper_for_level #49452

rhshadrach opened this issue Nov 1, 2022 · 14 comments · Fixed by #49690

Comments

@rhshadrach
Copy link
Member

#49373 removed its only use. Since it's implemented on Index as well (where it's still needed), I think we should raise NotImplementedError to prevent any hard-to-understand tracebacks.

@Himanshuwagh
Copy link
Contributor

can get more description?

@natmokval
Copy link
Contributor

take

@rhshadrach
Copy link
Member Author

rhshadrach commented Nov 3, 2022

@Himanshuwagh - I'd be happy to - is there a part that is unclear?

@natmokval - It looks like @TaylorChaseHunter is already preparing a PR for this issue.

@TaylorChaseHunter
Copy link

Oh I'm sorry, I was just kind of practicing the process -- I didn't realize it notified here. @natmokval please feel free to take it and I'll remove my commit.

@rhshadrach
Copy link
Member Author

Ah - no problem @TaylorChaseHunter. Thanks for letting us know!

@natmokval
Copy link
Contributor

Oops, my mistake. I am quite new to the process too. @TaylorChaseHunter please take the issue and don’t remove your commit.

@natmokval natmokval removed their assignment Nov 3, 2022
@codamuse
Copy link
Contributor

codamuse commented Nov 5, 2022

take

@codamuse
Copy link
Contributor

codamuse commented Nov 5, 2022

@rhshadrach I made changes off the main branch but tests failed. Pretty sure your changes in #49373 are necessary? If so, should I branch off your branch or wait for that to merge then cleanup? Thanks!

edit: clearly depends on #49373. I'll wait for that merge then PR

@rhshadrach
Copy link
Member Author

Exactly right @codamuse - that PR should be merged in the next few days, thanks for waiting.

@rhshadrach
Copy link
Member Author

@codamuse - #49373 has been merged.

@codamuse
Copy link
Contributor

codamuse commented Nov 8, 2022

@codamuse - #49373 has been merged.

Thanks for the heads up, I'll PR the change tomorrow.

@ilaiyengar
Copy link

Hi! I am a first time contributor and would love to help. Is there anything that I could possibly be assigned to or help out with?

@codamuse
Copy link
Contributor

codamuse commented Nov 9, 2022

@rhshadrach Sorry but I've been trying to get tests to pass locally and not having much success. I cloned a fresh repo from pandas-dev:main, setup my docker environment, cythonized, then ran tests from the panda-dev environment in conda and I get lots of fails even when trying different options with pytest. Could be something with my setup, I dug into a little bit but couldn't figure it out. I've decided to setup a draft PR in my fork for the change and see how the checks go there in case there's something strange happening in my environment. I'll keep checking locally, none of the tests are at all related to the change I made (removing that function in multi.py). I'll keep you posted.

@codamuse
Copy link
Contributor

codamuse commented Nov 9, 2022

@rhshadrach Sorry but I've been trying to get tests to pass locally and not having much success. I cloned a fresh repo from pandas-dev:main, setup my docker environment, cythonized, then ran tests from the panda-dev environment in conda and I get lots of fails even when trying different options with pytest. Could be something with my setup, I dug into a little bit but couldn't figure it out. I've decided to setup a draft PR in my fork for the change and see how the checks go there in case there's something strange happening in my environment. I'll keep checking locally, none of the tests are at all related to the change I made (removing that function in multi.py). I'll keep you posted.

encouragingly the CI tests all passed...I'll try to sort out my local environment but I think this PR should be okay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants