DOC/CLN: move NDFrame.groupby to (DataFrame|Series).groupby

[topper-123]

By moving this method up to dataFrame/Series, we get better doc strings and more precise type hints.

[WillAyd]

Makes sense. Can we get rid of the get_grouper function altogether as part of this?

[topper-123]

get_groupby is used in Resampler, so its used elsewhere.

[topper-123]

This is needed to silence mypy.

[jorisvandenbossche]

Could you do from pandas.core.groupby.generic import DataFrameGroupBy? That would make the signature a bit cleaner for end users ("grp_generic" is meaningless for them)

[topper-123]

That's not possible, because it would give circular import errors.

Maybe in the future, when we we're om min python 3.8, we can do from __future__ import annotations and get it to work, but that's a long way in the future.

EDIT: I've renamed "grp_generic" to "groupby_generic", which is a bit clearer.

[jorisvandenbossche]

Also not if you put it behind a if TYPE_CHECKING: ?

[jorisvandenbossche]

I actually also don't understand this aspect of importing: what's the difference between from pandas.core.groupby import generic vs from pandas.core.groupby.generic import DataFrameGroupBy? Both import the pandas.core.groupby.generic (and thus run all imports in that file), no?

[topper-123]

I'm not sure I can articulate it, but I understand it like this: If you attach a name to the new module (like you'd do in from pandas.core.groupby.generic import DataFrameGroupBy) you force the pandas.core.groupby.generic module to be run at import time. This causes an error, if the calling module (here frame.py) itself must be run before groupby.generic, casing a circular error.

By not importing a name (here DataFrameGroupby), running the module is delayed, and things work.

[WillAyd]

@topper-123 IIUC the get_groupby usage in Resample sends the object as an argument so with this refactor wouldn't we just be able to change code like:

grouped = get_groupby(subset, by=None, grouper=grouper, axis=self.axis)

To

subset.groupby(by=None, grouper=grouper, axis=self.axis)

? I think would be good to go all the way on that cleanup if so

[topper-123]

@WillAyd , good observation, I've removed get_groupby from the code base in the newest version.

I've also renamed grp_generic to groupby_generic to make it's content a bit clearer.

[topper-123]

@WillAyd , looking more into it, that doesn't work after all, because the signatures to get_groupby and (DataFrame|Series).groupby are different. E.g. .groupby does not have parameters grouper and exclusions etc.

I'll revert the removal of get_groupby.

[topper-123]

Is this ok to merge? Notice that DataFrame.groupby doesn't have a grouper argument, while DataFrame.GroupBy does, so calling the the two are not equivalent.

[WillAyd]

Hmm so I think would be nice if we replaced get_grouper and aligned signatures; as it currently stands I'm not really sure this adds much though by moving the functions from the base class to subclasses so +/-0 on the change but let's see what @jreback thinks

[jreback]

ok with aligning signatures but in a dedicated PR

[topper-123]

I've rebased the PR.

I'm not sure I understand how the signatures can be aligned: e.g. GroupBy has a grouper argument, which NDFrame.groupby doesn't have. I don't think it's a good idea to add the internal parameters of GroupBy to the public-facing NDFrame.groupby.

[jreback]

I've rebased the PR.

I'm not sure I understand how the signatures can be aligned: e.g. GroupBy has a grouper argument, which NDFrame.groupby doesn't have. I don't think it's a good idea to add the internal parameters of GroupBy to the public-facing NDFrame.groupby.

yeah I am now confused on what was said about aligning things. The Grouper and GroupBy are different things here.

thanks for the PR.