Skip to content

TYP: annotations in core.groupby #35939

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 21 commits into from
Aug 31, 2020
Merged

TYP: annotations in core.groupby #35939

merged 21 commits into from
Aug 31, 2020

Conversation

jbrockmendel
Copy link
Member

I'm still seeing a couple of mypy complaints, suggestions @simonjayhawkins ?

jbrockmendel and others added 19 commits August 20, 2020 21:19
@jbrockmendel
REF: remove unnecesary try/except
4c5eddd
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
c632c9f 
…f-blockwise-3
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
9e64be3 
…f-blockwise-3
@mathurk1 @jbrockmendel
TST: add test for agg on ordered categorical cols (pandas-dev#35630)
42649fb
@tkmz-n @jbrockmendel
TST: resample does not yield empty groups (pandas-dev#10603) (pandas-…
47121dd 
…dev#35799)
@jbrockmendel
revert accidental rebase
1decb3e
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
57c5dd3 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
a358463 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
ffa7ad7 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
e5e98d4 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
408db5a 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
d3493cf 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
75a805a 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
9f61070 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
2d10f6e 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
3e20187 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
e27d07f 
…ster
@jbrockmendel
Merge branch 'master' of https://github.com/pandas-dev/pandas into ma…
c52bed4 
…ster
@jbrockmendel
TYP: annotations in core.groupby
5f71a05
simonjayhawkins
simonjayhawkins reviewed Aug 28, 2020
View reviewed changes
pandas/core/groupby/generic.py Outdated
@@ -1638,7 +1638,7 @@ def _get_data_to_aggregate(self) -> BlockManager:
else:
return obj._mgr

def _insert_inaxis_grouper_inplace(self, result):
def _insert_inaxis_grouper_inplace(self, result: DataFrame):
Copy link
Member

Choose a reason for hiding this comment

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

for functions other than __init__ you can add None as the return type.

pandas/core/groupby/groupby.py
@@ -734,7 +734,7 @@ def pipe(self, func, *args, **kwargs):

plot = property(GroupByPlot)

def _make_wrapper(self, name):
def _make_wrapper(self, name: str) -> Callable:
Copy link
Member

Choose a reason for hiding this comment

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

add type parameters for Callable (if you can)

Copy link
Member Author

Choose a reason for hiding this comment

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

its going to end up being wrapper(self, *args, **kwargs) -> FrameOrSeriesUnion. Is that Callable[..., FrameOrSeriesUnion]?

Copy link
Member

Choose a reason for hiding this comment

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

sure

@simonjayhawkins
Copy link
Member

simonjayhawkins commented Aug 28, 2020 •
edited
Loading

I'm still seeing a couple of mypy complaints, suggestions @simonjayhawkins ?

pandas\core\groupby\categorical.py:101: error: "CategoricalIndex" has no attribute "set_categories"; maybe "categories"? [attr-defined]
pandas\core\groupby\categorical.py:104: error: "CategoricalIndex" has no attribute "add_categories"; maybe "categories"? [attr-defined]

These are because mypy is a static type checker and these methods are dynamically added to CategoricalIndex

in DatetimeIndex, tz_localize and to_period were added statically #34630 (comment)

I'm OK with # type: ignores on this pass (with error code)

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Aug 28, 2020
@simonjayhawkins
Copy link
Member

error: Argument 1 to "map" has incompatible type overloaded function; expected "Callable[[object], Iterator[_T]]" [arg-type]

not sure on this one.

mypy is happy with

        for name, lev, in_axis in zip(
            reversed(self.grouper.names),
            reversed(self.grouper.get_group_levels()),
            reversed([grp.in_axis for grp in self.grouper.groupings]),
        ):

which I think is easier to grok anyhow.

@simonjayhawkins simonjayhawkins added this to the 1.2 milestone Aug 28, 2020
@simonjayhawkins
Copy link
Member

simonjayhawkins commented Aug 28, 2020 •
edited
Loading

error: Argument 1 to "map" has incompatible type overloaded function; expected "Callable[[object], Iterator[_T]]" [arg-type]

not sure on this one.

see python/mypy#6811 (comment)

As for the originally reported bug, it also repros with this simpler code:

def get_common_index(*names: str) -> int:
   x = map(reversed, names)
   return 0

simonjayhawkins
simonjayhawkins reviewed Aug 28, 2020
View reviewed changes
pandas/core/groupby/generic.py Outdated
@@ -1848,6 +1848,7 @@ def nunique(self, dropna: bool = True):
],
axis=1,
)
assert isinstance(results, DataFrame) # for mypy
Copy link
Member

Choose a reason for hiding this comment

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

if you use cast here, then once we adopt Literal and concat can be overloaded on axis value, then with warn_redundant_casts we can remove

Copy link
Member Author

Choose a reason for hiding this comment

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

neat, will do

simonjayhawkins
simonjayhawkins reviewed Aug 28, 2020
View reviewed changes
pandas/core/groupby/grouper.py
Comment on lines +571 to +573
group_idx = self.group_index
assert isinstance(group_idx, CategoricalIndex) # set in __init__
return recode_from_groupby(self.all_grouper, self.sort, group_idx)
Copy link
Member

Choose a reason for hiding this comment

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

is this needed.

are the type annotations for def group_index(self) -> Index and _group_index: Optional[Index] = None correct.

in __init__

self._group_index = CategoricalIndex(..

or

(
                self.grouper,
                self._codes,
                self._group_index,
            ) = index._get_grouper_for_level(self.grouper, level)

so self._group_index can only be CategoricalIndex or None and group_index can only be CategoricalIndex ?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC its the self.all_grouper check a few lines up that ensures we have a CategoricalIndex here

Copy link
Member

Choose a reason for hiding this comment

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

self.all_grouper is not None when is_categorical_dtype(self.grouper) so I don't think that narrows it. (but I may need to trace through further)

simonjayhawkins
simonjayhawkins approved these changes Aug 30, 2020
View reviewed changes
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @jbrockmendel generally lgtm.

@simonjayhawkins simonjayhawkins merged commit 132e191 into pandas-dev:master Aug 31, 2020
@simonjayhawkins
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the annotate-groupby branch August 31, 2020 14:44
jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request Aug 31, 2020
@jbrockmendel
TYP: annotations in core.groupby (pandas-dev#35939)
3c841e8
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
@jbrockmendel
TYP: annotations in core.groupby (pandas-dev#35939)
a6440a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonjayhawkins simonjayhawkins simonjayhawkins approved these changes

Assignees
No one assigned
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@jbrockmendel @simonjayhawkins @mathurk1 @tkmz-n