Skip to content

BUG: REF: Groupby refactor #46754

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 19, 2022
Merged

BUG: REF: Groupby refactor #46754

merged 5 commits into from
Apr 19, 2022

Conversation

NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Apr 12, 2022 •
edited
Loading

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.

@NickCrews NickCrews mentioned this pull request Apr 12, 2022
Closed
1 task
@NickCrews
Copy link
Contributor Author

NickCrews commented Apr 13, 2022 •
edited
Loading

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.

@NickCrews
Copy link
Contributor Author

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?

@rhshadrach
Copy link
Member

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

@NickCrews
TYP: Add Literal annotations to TimeGrouper
f1ec0e5 
This doesn't affect the behaviour, since the possible values are
already restricted a few lines above this.
@NickCrews
TYP: Fix annotation of Grouper._set_grouper
f9cdbc6 
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.
@NickCrews
BUG: Include "dropna" in Grouper._attributes
3249b43 
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
Copy link
Contributor Author

NickCrews commented Apr 16, 2022 •
edited
Loading

@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 NickCrews changed the title Groupby refactor BUG: REF: Groupby refactor Apr 16, 2022
rhshadrach
rhshadrach requested changes Apr 16, 2022
View reviewed changes
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.

Thanks for the PR! Looks good, one request.

pandas/core/groupby/grouper.py
@@ -413,7 +413,6 @@ def _set_grouper(self, obj: NDFrame, sort: bool = False):
# "NDFrameT", variable has type "None")
self.obj = obj # type: ignore[assignment]
self._gpr_index = ax
return self._gpr_index
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Member

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).

Copy link
Contributor Author

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!

@rhshadrach rhshadrach added this to the 1.5 milestone Apr 16, 2022
@rhshadrach rhshadrach added the Output-Formatting __repr__ of pandas objects, to_string label Apr 16, 2022
jreback
jreback approved these changes Apr 17, 2022
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm, over to you @rhshadrach

@NickCrews
Copy link
Contributor Author

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

rhshadrach
rhshadrach approved these changes Apr 19, 2022
View reviewed changes
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 merged commit bbac28f into pandas-dev:main Apr 19, 2022
@jreback
Copy link
Contributor

jreback commented Apr 19, 2022

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

@jreback
Copy link
Contributor

jreback commented Apr 19, 2022

oh nvm

i was looking at an older version

thanks @NickCrews

@rhshadrach
Copy link
Member

Thanks @NickCrews!

yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
@NickCrews @yehoshuadimarsky
BUG: Add dropna to Grouper repr (pandas-dev#46754)
4a11161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

@rhshadrach rhshadrach rhshadrach approved these changes

Assignees
No one assigned
Labels
Groupby Output-Formatting __repr__ of pandas objects, to_string
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.
3 participants
@NickCrews @rhshadrach @jreback