BUG: REF: Groupby refactor by NickCrews · Pull Request #46754 · pandas-dev/pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits5 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This is a follow-up, scaled back version of #46258, that hopefully should be easier to merge.

• [NA, this is small ] closes #xxxx (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 an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

I added a merge commit again because the CI run seemed to be failing because it was using an old version of the PR, before I updated the tests? Anyways, this PR seems to be innocent of any failing tests.

OK, some of the tests (https://github.com/pandas-dev/pandas/runs/6011207511?check_suite_focus=true) seem to still not be picking up the changed tests and are failing? But some of them have picked up the modified tests and are therefore passing? Not sure what this means, is the CI being weird or does this actually mean I need to adjust the tests more because I'm missing something?

@NickCrews - does the one failing test pass for you locally?

This doesn't affect the behaviour, since the possible values are already restricted a few lines above this.

The return value is never used. Don't pretend that it is.

Ideally we don't even hold this state inside the Grouper, it would make it easier to reason about. But just do this tiny change for now, still an improvement. See pandas-dev#46258 for more details.

All this changes I think is that now dropna is included in the __repr__ output. See pandas-dev#46258 for more discussion.

And move dropna up out of the "generated" attribute section into the "source" attribute section in the init

@NickCrews - does the one failing test pass for you locally?

Poking into this showed that there was a second assert in the test case that I hadn't noticed and updated 🤦 . Looks good locally now, probably CI will pass.

NickCrews changed the titleGroupby refactor BUG: REF: Groupby refactor

Apr 16, 2022

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good, one request.

Choose a reason for hiding this comment

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

This seems okay to me; the result of this method is indeed never used and it looks cleaner to not have a return. cc @jbrockmendel

Choose a reason for hiding this comment

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

Thanks for the review @rhshadrach! FYI we talked about this a bit at #46258 (comment) if you want a bit more context

Choose a reason for hiding this comment

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

Marking as resolved, since I'm worried the "changes requested" label might be keeping this out of your queue.

Choose a reason for hiding this comment

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

I don't think it being resolved or not changes its appearance in e.g. pandas/pulls or the GitHub notifications (not sure what else you might mean by queue here).

Choose a reason for hiding this comment

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

That is what I meant (maybe you had filters for what to review next), but good to know. Thanks!

Choose a reason for hiding this comment

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

little rebase to fix a merge conflict that appeared from the whatsnew changing in main

Choose a reason for hiding this comment

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

lgtm

hmm i think this needs to merge master once more
@NickCrews

oh nvm

i was looking at an older version

thanks @NickCrews

yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request

Jul 13, 2022