TYP: Groupby sum|prod|min|max|first|last methods #32302

topper-123 · 2020-02-27T18:14:32Z

This makes two things:

makes mypy know the signatures of the agg methods (sum, prod, etc.) on Groupby
gives the agg methods on Groupby objects a return type

This ensues that mypy and IDE's always know that the return type of e.g. df.groupby('a').sum() is a DataFrame, which is nice when chaining.

WillAyd

On board with the concept; a few things

pandas/core/groupby/generic.py

WillAyd · 2020-02-27T19:50:00Z

pandas/core/groupby/generic.py

@@ -789,6 +811,36 @@ def count(self) -> Series:
        )
        return self._reindex_output(result, fill_value=0)

+    @Substitution(f="sum", no=True, mc=0, return_type="Series")
+    @Appender(_agg_template)
+    def sum(self, numeric_only=True, min_count=0) -> Series:


Can you annotate numeric_only and min_count on these as well?

pandas/util/_decorators.py

WillAyd

lgtm

jbrockmendel · 2020-02-28T02:24:10Z

there's got to be a less verbose way of doing this, isnt there?

jreback

why are you not generating these like we do in genetic for the stat functions ?

topper-123 · 2020-02-28T12:57:26Z

why are you not generating these like we do in genetic for the stat functions ?

Doing e.g. reveal_type(df.var)on master gives a revealed type of Any. I don't think it's possible to programatically add methods and also have mypy infer their types, parameters or return values. Mypy isn't advance enough to do that.

It's true that this is a bit repetitive, but always with slight variations. Getting correct parameter and return types is something I value highly in a library as complex as Pandas, so IMO this is worth it.

jbrockmendel · 2020-02-28T16:07:35Z

I don't think it's possible to programatically add methods and also have mypy infer their types, parameters or return values. Mypy isn't advance enough to do that.

cc @simonjayhawkins is this likely to improve at any point? would something like stub files help?

I find this pattern annoying at debug-time, when it is an extra layer of calls to look through (since I have to double-check whether the super().foo(...) call is changing any kwargs).

(core.window.expanding is another place where we do something like this, but in that case for docstrings)

WillAyd · 2020-02-29T00:46:23Z

FWIW I don't find stepping through the functions as is necessarily any clearer than inheriting as done here. This is more verbose, but I don't think factory-generating these really adds all that much value and I do think would be nice to support annotations on these

simonjayhawkins · 2020-03-02T12:43:31Z

pandas/core/groupby/generic.py

+    @doc(_agg_template, fname="last", no=False, mc=-1, return_type="DataFrame")
+    def last(self, numeric_only: bool = False, min_count: int = -1) -> DataFrame:
+        return super().last(numeric_only=numeric_only, min_count=min_count)
+


These are duplicates from above except the return values. Can you use Generic instead? The exception would be for unique docstrings for the Series/Frame methods. Does something need to be added to make these docstrings visible in the documentation.

The doc string parameter return_type is also different, though. I could change these into e.g.:

@doc(_agg_template, fname="last", no=False, mc=-1, return_type="DataFrame") def last(self, numeric_only: bool = False, min_count: int = -1) -> DataFrame: last_compat = partial(self._get_loc, loc=-1) return self._agg_general( numeric_only=numeric_only, min_count=min_count, alias="last", npfunc=last_compat, )

on both SeriesGroupby and DataFrameGroupby (setting 'Dataframe' and 'Series' the relevant places). That would be repetitipus also, though, but it would avoid setting methods on the NDFrame class...

sorry if I wasn't clear, in the docs I see https://pandas.io/docs/reference/api/pandas.core.groupby.GroupBy.min.html but nothing for DataFrameGroupBy.min, whereas for most other methods there is a DataFrameGroupBy.

So my question is, how to make these visible, otherwise if the generic docstring is used, this duplication is unnecessary and GroupBy could be Generic to give the correct return type. see https://github.com/pandas-dev/pandas/pull/29480/files#r347723361 where the idea was rejected.

Ok, I see your point now, it's the issue of the doc reference.

I don't have a good solution to that. I've added doc strings to GroupBy methods and copied them over to SeriesGroupby and DataFrameGroupby. SeriesGroupby and DataFrameGroupby need methods themselves in order to get the correct type hint. feels repetitive, but I don't think we can make it better without dropping the type hint.

simonjayhawkins · 2020-03-04T14:03:16Z

there's got to be a less verbose way of doing this, isnt there?

@topper-123 commit making _Groupby a generic class. dccbd00

jreback

I would just add all of the functions here and avoid changing agg_general at all, where the changes you are attempting are non-trivial (do a pre-cursor PR).

ok conceptually with adding these methods to get better typing.

jreback · 2020-03-04T14:18:24Z

pandas/core/groupby/groupby.py

+        )
+
+    @staticmethod
+    def _get_loc(x, axis: int = 0, *, loc: int):


what?

a) don't use staticmethod
b) what the heck is this for

I would be more ok with this if its not a nested function like this. I am also -1 on any use of staticmethod.

jreback · 2020-03-04T14:18:37Z

pandas/core/groupby/groupby.py

-            numeric_only: bool = True,
-            min_count: int = -1,
-        ):
+        def get_loc_notna(x, loc: int):


what is this?

jreback

can you split this pr up

a PR to refactor groupby_function so its clear what is going on
the typing of functions

WillAyd · 2020-03-27T22:09:44Z

@topper-123 still active? I think this is a nice change and helpful when we think about bumping our mypy pin, so would love if there's an easy way to address comments

topper-123 · 2020-03-28T14:53:41Z

I've been a bit busy lately. I can see if I can make something work over the coming days.

jreback · 2020-04-07T00:14:34Z

can you merge master ping on green.

topper-123 · 2020-05-08T11:47:45Z

I've updated the PR and removed the return types, so this is only refactoring _add_numeric_operations and make mypy discover the methods.

So, this does:

>>> meth = df.groupby('x').sum
>>> reveal_type(meth)
Any  # master
Revealed type is 'def (numeric_only: builtins.bool =, min_count: builtins.int =) -> Any'  # this PR

The Travis failure looks unrelated.

jreback · 2020-05-09T19:19:12Z

pandas/core/groupby/groupby.py

+        )
+
+    @staticmethod
+    def _get_loc(x, axis: int = 0, *, loc: int):


I would be more ok with this if its not a nested function like this. I am also -1 on any use of staticmethod.

topper-123 · 2020-05-15T05:31:08Z

I've made a new commit that simplifies the PR wrt. the static method.

topper-123 · 2020-05-15T21:21:18Z

Superceded by #34200.

topper-123 changed the title ~~CLN/TYP: Groupby agg methods~~ TYP: Groupby agg methods Feb 27, 2020

WillAyd requested changes Feb 27, 2020

View reviewed changes

WillAyd added Groupby Typing type annotations, mypy/pyright type checking labels Feb 27, 2020

topper-123 force-pushed the groupby_agg_methods branch from d872b8c to 81377e9 Compare February 27, 2020 21:26

WillAyd requested changes Feb 27, 2020

View reviewed changes

pandas/util/_decorators.py Show resolved Hide resolved

WillAyd approved these changes Feb 27, 2020

View reviewed changes

jreback requested changes Feb 28, 2020

View reviewed changes

simonjayhawkins reviewed Mar 2, 2020

View reviewed changes

topper-123 force-pushed the groupby_agg_methods branch 4 times, most recently from 5247c92 to d47c819 Compare March 2, 2020 23:06

jreback requested changes Mar 4, 2020

View reviewed changes

jreback requested changes Mar 14, 2020

View reviewed changes

simonjayhawkins mentioned this pull request Apr 5, 2020

CLN: Add/refine type hints to some functions in core.dtypes.cast #33286

Merged

5 tasks

topper-123 force-pushed the groupby_agg_methods branch from d47c819 to 292896f Compare May 8, 2020 10:56

topper-123 force-pushed the groupby_agg_methods branch from 292896f to 14b071b Compare May 9, 2020 17:55

jreback requested changes May 9, 2020

View reviewed changes

CLN/TYP: Groupby agg methods

3357d06

topper-123 added 2 commits May 14, 2020 18:36

add parameter types + @doc decorator

4b96a52

Add doc string to GroupBy methods

be54f6c

topper-123 force-pushed the groupby_agg_methods branch from 14b071b to 34fd4b8 Compare May 15, 2020 05:29

simplify first_compat and last_compat

279a89e

topper-123 force-pushed the groupby_agg_methods branch from 34fd4b8 to 279a89e Compare May 15, 2020 06:04

topper-123 closed this May 15, 2020

topper-123 deleted the groupby_agg_methods branch May 15, 2020 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TYP: Groupby sum|prod|min|max|first|last methods #32302

TYP: Groupby sum|prod|min|max|first|last methods #32302

topper-123 commented Feb 27, 2020 •

edited

Loading

WillAyd left a comment

WillAyd Feb 27, 2020

WillAyd left a comment

jbrockmendel commented Feb 28, 2020

jreback left a comment

topper-123 commented Feb 28, 2020

jbrockmendel commented Feb 28, 2020

WillAyd commented Feb 29, 2020

simonjayhawkins Mar 2, 2020

topper-123 Mar 2, 2020 •

edited

Loading

simonjayhawkins Mar 2, 2020

topper-123 Mar 2, 2020

simonjayhawkins commented Mar 4, 2020

jreback left a comment

jreback Mar 4, 2020

jreback May 9, 2020

jreback Mar 4, 2020

jreback left a comment

WillAyd commented Mar 27, 2020

topper-123 commented Mar 28, 2020

jreback commented Apr 7, 2020

topper-123 commented May 8, 2020

jreback May 9, 2020

topper-123 commented May 15, 2020

topper-123 commented May 15, 2020

TYP: Groupby sum|prod|min|max|first|last methods #32302

TYP: Groupby sum|prod|min|max|first|last methods #32302

Conversation

topper-123 commented Feb 27, 2020 • edited Loading

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 28, 2020

jreback left a comment

Choose a reason for hiding this comment

topper-123 commented Feb 28, 2020

jbrockmendel commented Feb 28, 2020

WillAyd commented Feb 29, 2020

Choose a reason for hiding this comment

topper-123 Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins commented Mar 4, 2020

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

WillAyd commented Mar 27, 2020

topper-123 commented Mar 28, 2020

jreback commented Apr 7, 2020

topper-123 commented May 8, 2020

Choose a reason for hiding this comment

topper-123 commented May 15, 2020

topper-123 commented May 15, 2020

topper-123 commented Feb 27, 2020 •

edited

Loading

topper-123 Mar 2, 2020 •

edited

Loading