Skip to content

Clean up Abstract and Naming Definitions for GroupBy #28847

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
merged 6 commits into from
Oct 8, 2019

Conversation

WillAyd
Copy link
Member

@WillAyd WillAyd commented Oct 8, 2019

Summary of changes:

  • Made _iterate_slices abstract in base class, moved pre-existing definition from Base -> Series where it was actually being called (DataFrame already overrides)
  • Made _wrap_transformed_output Abstract in base class, as both Series and DataFrame override this
  • Redefined _wrap_applied_output; this was Abstract in base class before but its definition was not in sync with how the subclasses actually defined
  • Renamed _wrap_output to _wrap_series_output as it is only valid for SeriesGroupBy
  • Renamed _wrap_generic_output to _wrap_frame_output as it is only valid for DataFrameGroupBy
  • Renamed _aggregate_generic to _aggregate_frame as it is only valid for DataFrameGroupBy

More to come in separate PRs

@WillAyd WillAyd added Refactor Internal refactoring of code Groupby labels Oct 8, 2019
@pep8speaks
Copy link

pep8speaks commented Oct 8, 2019

Hello @WillAyd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-08 16:15:58 UTC

@jbrockmendel
Copy link
Member

makes sense. LGTM pending green

@WillAyd WillAyd added this to the 1.0 milestone Oct 8, 2019
@jreback jreback merged commit df2e081 into pandas-dev:master Oct 8, 2019
@jreback
Copy link
Contributor

jreback commented Oct 8, 2019

thanks @WillAyd

doc-strings would be very helpful on some of these as well :->

@WillAyd WillAyd deleted the groupby-abstracts branch October 8, 2019 21:25
@WillAyd
Copy link
Member Author

WillAyd commented Oct 8, 2019

Agreed - docstrings and annotations should come in one of these

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groupby Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants