Remove NDFrameGroupBy Class #28835
Remove NDFrameGroupBy Class #28835
Conversation
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
you know how we love large PRs!
but will look
|
Yep...like I said I think the way git is calculating the diff on this makes it seem a lot bigger than it is. Mostly just a cut and paste of the entire class def
…Sent from my iPhone
On Oct 8, 2019, at 3:18 AM, Jeff Reback ***@***.***> wrote:
@jreback requested changes on this pull request.
you know how we love large PRs!
but will look
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
where is this NOT a cut-paste? can you comment on the code |
| axis="", | ||
| ) | ||
| @Appender(_shared_docs["aggregate"]) | ||
| def aggregate(self, func=None, *args, **kwargs): |
There was a problem hiding this comment.
aggregate was not a copy paste because there was a def in both classes. However, the current master for DataFrameGroupBy just calls super, so that function body was simply replaced with what was previously in NDFrameGroupBy
|
|
||
| return self._wrap_transformed_output(output, names) | ||
|
|
||
| def _wrap_aggregated_output(self, output, names=None): |
There was a problem hiding this comment.
This func was also not a direct copy paste because it existed in both classes. What was in NDFrameGroupBy was moved to GroupBy (can ref last commit)
|
It might also be helpful to look at the commits besides the blackify one to see the only things that aren’t direct copy paste. The initial commit is the entire cut and paste, just difficult to see that by default with git |
|
thanks @WillAyd copy-pasting is ok here! yeah this makes sense.. |
3 participants
Was going to do a larger refactor but this diff looks more confusing than it actually is, so figured I'd stop here for readability. All I've done in this PR is:
After this I am looking to find a more logical home for the functions, as the current hierarchy isn't super clear. For example,
_iterate_slices()should probably be abstract in the baseGroupByclass, but right now the Series implementation is in the superclass while DataFrameGroupBy overrides that for itself. Series.pct_change is practically the same as GroupBy.pct_change, so is probably unnecessary. Will be a few more follow ups mixed in