TYP: handle mypy ignore in SelectionMixin #38239

ivanovmg · 2020-12-02T17:17:27Z

xref TYP: investigate/fix ignored mypy errors #37715
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Handle mypy ignore comments in pandas/core/base.py, class SelectionMixin.
There were issues with attributes obj and exclusions not defined in the mixin, but defined in the child classes.

I wish I made obj of type FrameOrSeries rather than Any.
This is the type used in the child classes, but this results in multiple mypy errors of this kind.

pandas\core\window\rolling.py:197: error: FrameOrSeries? has no attribute "ndim"  [attr-defined]

If I go with FrameOrSeriesUnion, then it is necessary to go and do casting where required to DataFrame or Series.
This is necessary since, for example, some kwargs for either of the types are irrelevant.

jbrockmendel · 2020-12-02T17:48:26Z

pandas/core/base.py

@@ -152,6 +154,9 @@ class SelectionMixin:
    object sub-classes need to define: obj, exclusions
    """

+    obj: Any
+    exclusions: Set[Optional[Hashable]] = set()


Right! Thank you.

ivanovmg · 2020-12-03T08:34:16Z

I am not sure about the problem, but docs build finished with two warnings, causing CI to fail.

build finished with problems, 2 warnings.
Error: Process completed with exit code 1.

jbrockmendel · 2020-12-07T22:46:41Z

pandas/core/base.py

@@ -152,6 +153,9 @@ class SelectionMixin:
    object sub-classes need to define: obj, exclusions
    """

+    obj: Any
+    exclusions: Set[Label] = set()


looks like the docbuild failure is bc this is public but undocumented

The docstring states exclusions : set, optional. Is the docstring referring to required class attributes? The docstring states sub-classes need to define: obj, exclusions. why is exclusions initialised here?

exclusions is initialized here to deal with the following mypy errors.

pandas\core\base.py:230: error: "SelectionMixin" has no attribute "exclusions" [attr-defined] pandas\core\base.py:231: error: "SelectionMixin" has no attribute "exclusions" [attr-defined]

no need to initialize if sub-classes need to define as stated in the docstring, declaration will silence those errors.

Suggested change

exclusions: Set[Label] = set()

exclusions: Set[Label]

ivanovmg · 2020-12-10T09:07:39Z

I documented obj and exclusions in SelectionMixin docstring.
But there are still complains in build docs.

/home/runner/work/pandas/pandas/doc/source/reference/api/pandas.DataFrame.exclusions.rst: WARNING: document isn't included in any toctree
/home/runner/work/pandas/pandas/doc/source/reference/api/pandas.Series.exclusions.rst: WARNING: document isn't included in any toctree

Any idea how to fix it?

jbrockmendel · 2020-12-10T16:52:09Z

i was thinking just make them private

ivanovmg · 2020-12-11T19:24:42Z

i was thinking just make them private

I tried, but the thing is that they are used heavily in the classes (at least obj), which subclass from SelectionMixin.
I guess, than avoiding mypy errors by making an actively used attributes private and thus changing quite the code base, is too costly.

jreback · 2020-12-22T00:24:19Z

can you merge master

simonjayhawkins

Thanks @ivanovmg for the PR

I wish I made obj of type FrameOrSeries rather than Any.

If the type of self.obj does not change during it's lifetime, then this is the correct annotation

If I go with FrameOrSeriesUnion

This would be correct if the type of self.obj is allowed to change.

simonjayhawkins · 2020-12-28T17:10:33Z

pandas/core/base.py

    object sub-classes need to define: obj, exclusions
+
+    obj : FrameOrSeries


we tend not to use the typing aliases in docstrings

simonjayhawkins · 2020-12-28T17:11:26Z

pandas/core/base.py

@@ -148,10 +149,18 @@ class SpecificationError(Exception):

 class SelectionMixin:
    """
-    mixin implementing the selection & aggregation interface on a group-like
+    Mixin implementing the selection & aggregation interface on a group-like


when making changes to docstrings, if possible can you make the first line a one-liner.

Made as a one-liner.

simonjayhawkins · 2020-12-28T17:14:22Z

pandas/core/base.py

    """

+    obj: Any


obj here is Any and FrameOrSeries in docstring.

Yes, this looks odd.
Initially I typed obj as FrameOrSeries, but then mypy would raise errors of this kind.

pandas\core\window\rolling.py:228: error: FrameOrSeries? has no attribute "ndim" [attr-defined] pandas\core\window\rolling.py:229: error: FrameOrSeries? has no attribute "__iter__" (not iterable) [attr-defined] pandas\core\window\rolling.py:236: error: FrameOrSeries? has no attribute "__iter__" (not iterable) [attr-defined] pandas\core\window\rolling.py:244: error: FrameOrSeries? has no attribute "_dir_additions" [attr-defined] pandas\core\window\rolling.py:324: error: FrameOrSeries? has no attribute "index" [attr-defined]

The reason is that DataFrame and Series interface is similar, but different.
Some attributes are defined in one, but not defined in another, or the methods have different signatures.

So, I had to fallback with Any.

FrameOrSeries is a typevar, so to use it on a class/instance variable requires to make the SelectionMixin class generic. The mypy error suggests this.

pandas/core/base.py:161: error: Type variable "pandas._typing.FrameOrSeries" is unbound [valid-type] pandas/core/base.py:161: note: (Hint: Use "Generic[FrameOrSeries]" or "Protocol[FrameOrSeries]" base class to bind "FrameOrSeries" inside a class) pandas/core/base.py:161: note: (Hint: Use "FrameOrSeries" in function signature to bind "FrameOrSeries" inside a function)

if having self.obj not maintain it's type for the duration of the instance life is ok, then use FrameOrSeriesUnion instead.

mypy errors such as pandas/core/window/rolling.py:108: error: Incompatible types in assignment (expression has type "FrameOrSeries", variable has type "Union[DataFrame, Series]") [assignment] if doing this are where FrameOrSeries aliases are used elsewhere that also would need to change to FrameOrSeriesUnion.

There are 34 mypy errors of this kind (see details below) when setting to FrameOrSeriesUnion.
Would it be a good idea for the follow up PR to fix them?

pandas\core\window\rolling.py:108: error: Incompatible types in assignment (expression has type "FrameOrSeries", variable has type "Union[DataFrame, Series]") [assignment] pandas\core\groupby\groupby.py:571: error: Incompatible types in assignment (expression has type "FrameOrSeries", variable has type "Union[DataFrame, Series]") [assignment] pandas\core\groupby\groupby.py:872: error: Argument 1 to "get_iterator" of "BaseGrouper" has incompatible type "Union[DataFrame, Series]"; expected "FrameOrSeries" [arg-type] pandas\core\groupby\generic.py:233: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:358: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:440: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:467: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:501: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:567: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:567: error: Incompatible return value type (got "Union[DataFrame, Series]", expected "Series") [return-value] pandas\core\groupby\generic.py:668: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:669: error: Incompatible return value type (got "Union[DataFrame, Series]", expected "Series") [return-value] pandas\core\groupby\generic.py:777: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:809: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:828: error: Unexpected keyword argument "name" for "DataFrame" [call-arg] pandas\core\frame.py:511: note: "DataFrame" defined here pandas\core\groupby\generic.py:834: error: Incompatible return value type (got "Union[DataFrame, Series]", expected "Series") [return-value] pandas\core\groupby\generic.py:947: error: Unexpected keyword argument "columns" for "Series" [call-arg] pandas\core\series.py:218: note: "Series" defined here pandas\core\groupby\generic.py:1171: error: Unexpected keyword argument "columns" for "Series" [call-arg] pandas\core\series.py:218: note: "Series" defined here pandas\core\groupby\generic.py:1171: error: Incompatible return value type (got "Union[DataFrame, Series]", expected "DataFrame") [return-value] pandas\core\groupby\generic.py:1278: error: Unexpected keyword argument "columns" for "Series" [call-arg] pandas\core\series.py:218: note: "Series" defined here pandas\core\groupby\generic.py:1289: error: Argument 1 to "_insert_inaxis_grouper_inplace" of "DataFrameGroupBy" has incompatible type "Union[DataFrame, Series]"; expected "DataFrame" [arg-type] pandas\core\groupby\generic.py:1325: error: Unexpected keyword argument "columns" for "Series" [call-arg] pandas\core\series.py:218: note: "Series" defined here pandas\core\groupby\generic.py:1353: error: Unexpected keyword argument "columns" for "Series" [call-arg] pandas\core\series.py:218: note: "Series" defined here pandas\core\groupby\generic.py:1398: error: Item "type" of "Union[Type[DataFrame], Type[Series]]" has no attribute "_from_arrays" [union-attr] pandas\core\groupby\generic.py:1462: error: Unexpected keyword argument "columns" for "Series" [call-arg] pandas\core\series.py:218: note: "Series" defined here pandas\core\groupby\generic.py:1462: error: Incompatible return value type (got "Union[DataFrame, Series]", expected "DataFrame") [return-value] pandas\core\groupby\generic.py:1597: error: Unexpected keyword argument "columns" for "Series" [call-arg] pandas\core\series.py:218: note: "Series" defined here pandas\core\groupby\generic.py:1601: error: Unexpected keyword argument "columns" for "Series" [call-arg] pandas\core\series.py:218: note: "Series" defined here pandas\core\groupby\generic.py:1601: error: Incompatible return value type (got "Union[DataFrame, Series]", expected "DataFrame") [return-value] pandas\core\groupby\generic.py:1648: error: Argument 1 to "_insert_inaxis_grouper_inplace" of "DataFrameGroupBy" has incompatible type "Union[DataFrame, Series]"; expected "DataFrame" [arg-type] pandas\core\groupby\generic.py:1656: error: Incompatible return value type (got "Union[DataFrame, Series]", expected "DataFrame") [return-value] pandas\core\groupby\generic.py:1686: error: Incompatible return value type (got "Union[DataFrame, Series]", expected "DataFrame") [return-value] pandas\core\groupby\generic.py:1694: error: Argument 1 to "_insert_inaxis_grouper_inplace" of "DataFrameGroupBy" has incompatible type "Union[DataFrame, Series]"; expected "DataFrame" [arg-type] pandas\core\groupby\generic.py:1704: error: Incompatible return value type (got "Union[DataFrame, Any, Series]", expected "DataFrame") [return-value]

I think I can do them here, but first need to figure out how to deal with "CI Web and docs", since it complains about the missing documentation

2021-01-10T11:33:04.9021813Z /home/runner/work/pandas/pandas/doc/source/reference/api/pandas.DataFrame.exclusions.rst: WARNING: document isn't included in any toctree 2021-01-10T11:33:04.9024422Z /home/runner/work/pandas/pandas/doc/source/reference/api/pandas.Series.exclusions.rst: WARNING: document isn't included in any toctree

Would it be a good idea for the follow up PR to fix them?

put a big TODO by the variable declaration otherwise this could get missed. Any should only be used where we want to allow dynamic typing ( and lose the benefits of static type checking)

simonjayhawkins · 2020-12-28T17:25:37Z

pandas/core/base.py

@@ -152,6 +153,9 @@ class SelectionMixin:
    object sub-classes need to define: obj, exclusions
    """

+    obj: Any
+    exclusions: Set[Label] = set()


The docstring states exclusions : set, optional. Is the docstring referring to required class attributes? The docstring states sub-classes need to define: obj, exclusions. why is exclusions initialised here?

github-actions · 2021-02-11T00:12:45Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

simonjayhawkins

Thanks @ivanovmg for working on this.

There are many type: ignores that are being removed here, which is good. But I do have reservations about silencing the errors using Any. This is not really addressing the problem, just making it less visible.

Could you look into actually fixing this in this PR. We could then decide to split into separate PRs if necessary.

simonjayhawkins · 2021-02-11T15:03:15Z

pandas/core/base.py

    """

+    obj: Any
+    """Target object for the selection and aggregation."""


remove or change to comment

simonjayhawkins · 2021-02-11T15:03:25Z

pandas/core/base.py

+    # however this creates multiple mypy errors elsewhere.
+    # Those have to be addressed in a separate PR.
+    exclusions: Set[Hashable]
+    """Columns to exclude."""


simonjayhawkins · 2021-02-11T15:04:47Z

pandas/core/base.py

+    obj: Any
+    """Target object for the selection and aggregation."""
+    # GH 38239
+    # TODO obj here must be typed as FrameOrSeriesUnion,


Suggested change

# TODO obj here must be typed as FrameOrSeriesUnion,

# TODO: obj here must be typed as FrameOrSeriesUnion,

simonjayhawkins · 2021-02-11T15:05:28Z

pandas/core/base.py

+    # however this creates multiple mypy errors elsewhere.
+    # Those have to be addressed in a separate PR.


I don't think this adds value. can remove

pandas/core/base.py

Co-authored-by: Simon Hawkins <[email protected]>

jreback · 2021-02-15T23:58:44Z

can you merge master

pep8speaks · 2021-02-17T01:28:08Z

Hello @ivanovmg! 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 2021-03-16 06:40:20 UTC

jreback · 2021-02-17T01:34:53Z

@ivanovmg well my browser change added some whitespace :-< if you can fix up and push when you can.

simonjayhawkins · 2021-03-14T10:32:50Z

@ivanovmg can you merge master and fixup mypy failures.

ivanovmg · 2021-03-16T08:25:11Z

@simonjayhawkins I merged master.
It turns out that the changes introduced here regarding typing induce the following issues:

pandas/core/groupby/generic.py:1557: error: Unexpected keyword argument "columns" for "Series"  [call-arg]
pandas/core/groupby/generic.py:852: error: Unexpected keyword argument "name" for "DataFrame"  [call-arg]

In my opinion, there is a problem with the interchangeability of Series and DataFrame.
Fixing these problems is going to be extremely intrusive.
Considering the trade offs, IMHO, it is better to close this PR.

jbrockmendel · 2021-03-28T15:58:18Z

Considering the trade offs, IMHO, it is better to close this PR.

That's your call.

ivanovmg · 2021-03-28T16:23:32Z

Closing.

TYP: handle mypy ignore in SelectionMixin

34a6644

ivanovmg requested a review from simonjayhawkins December 2, 2020 17:18

jbrockmendel reviewed Dec 2, 2020

View reviewed changes

ivanovmg added 3 commits December 3, 2020 00:50

FIX: remove unnecessary import

19f0db3

TYP: use Label instead of Optional[Hashable]

d4a8a7e

Merge branch 'master' into mypy/base

b4dc30f

jbrockmendel reviewed Dec 7, 2020

View reviewed changes

ivanovmg added 3 commits December 8, 2020 10:51

Merge branch 'master' into mypy/base

48f2b08

DOC: document public attributes in SelectionMixin

bc5157d

Merge branch 'master' into mypy/base

74f6d69

Merge branch 'master' into mypy/base

a68c880

jreback added the Typing type annotations, mypy/pyright type checking label Dec 22, 2020

jreback added this to the 1.3 milestone Dec 22, 2020

Merge branch 'master' into mypy/base

68a3310

simonjayhawkins reviewed Dec 28, 2020

View reviewed changes

ivanovmg added 2 commits January 10, 2021 18:14

Merge branch 'master' into mypy/base

a1df593

REF: initialize exclusions in derived class

a99be97

github-actions bot added the Stale label Feb 11, 2021

ivanovmg added 4 commits February 11, 2021 18:52

TYP: TODO on typing obj as FrameOrSeriesUnion

dcc6885

Merge branch 'master' into mypy/base

3e025a7

DOC: doc attributes inside class body

a56f7b9

TYP: replace Label with Hashable

e0b1657

simonjayhawkins reviewed Feb 11, 2021

View reviewed changes

simonjayhawkins removed the Stale label Feb 11, 2021

Update pandas/core/base.py

4222c83

Co-authored-by: Simon Hawkins <[email protected]>

Merge branch 'master' into mypy/base

eb04519

ivanovmg force-pushed the mypy/base branch from d956660 to 2b61d4f Compare February 16, 2021 03:53

TYP: change to FrameOrSeriesUnion

954a2a3

ivanovmg force-pushed the mypy/base branch from 2b61d4f to 954a2a3 Compare February 16, 2021 04:07

Merge branch 'master' into mypy/base

9f703f1

LINT: fix trailing whitespace

e7f9028

ivanovmg force-pushed the mypy/base branch from 548de4a to e7f9028 Compare February 17, 2021 09:20

Merge branch 'master' into mypy/base

219eb11

ivanovmg closed this Mar 28, 2021

		object sub-classes need to define: obj, exclusions

		obj : FrameOrSeries

	# TODO obj here must be typed as FrameOrSeriesUnion,
	# TODO: obj here must be typed as FrameOrSeriesUnion,

		# however this creates multiple mypy errors elsewhere.
		# Those have to be addressed in a separate PR.

TYP: handle mypy ignore in SelectionMixin #38239

TYP: handle mypy ignore in SelectionMixin #38239

Conversation

ivanovmg commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanovmg commented Dec 3, 2020

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

ivanovmg commented Dec 10, 2020

jbrockmendel commented Dec 10, 2020

ivanovmg commented Dec 11, 2020

jreback commented Dec 22, 2020

simonjayhawkins 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

Choose a reason for hiding this comment

ivanovmg Jan 10, 2021 • edited Loading

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

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2021

simonjayhawkins 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

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 commented Feb 15, 2021

pep8speaks commented Feb 17, 2021 • edited Loading

Comment last updated at 2021-03-16 06:40:20 UTC

jreback commented Feb 17, 2021

simonjayhawkins commented Mar 14, 2021

ivanovmg commented Mar 16, 2021

jbrockmendel commented Mar 28, 2021

ivanovmg commented Mar 28, 2021

ivanovmg Jan 10, 2021 •

edited

Loading

pep8speaks commented Feb 17, 2021 •

edited

Loading