-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
TYP: handle mypy ignore in SelectionMixin #38239
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.
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
Changes from 4 commits
34a6644
19f0db3
d4a8a7e
b4dc30f
48f2b08
bc5157d
74f6d69
a68c880
68a3310
a1df593
a99be97
dcc6885
3e025a7
a56f7b9
e0b1657
4222c83
eb04519
954a2a3
9f703f1
e7f9028
219eb11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ | |||||
Dict, | ||||||
FrozenSet, | ||||||
Optional, | ||||||
Set, | ||||||
TypeVar, | ||||||
Union, | ||||||
cast, | ||||||
|
@@ -19,7 +20,7 @@ | |||||
import numpy as np | ||||||
|
||||||
import pandas._libs.lib as lib | ||||||
from pandas._typing import DtypeObj, IndexLabel | ||||||
from pandas._typing import DtypeObj, IndexLabel, Label | ||||||
from pandas.compat import PYPY | ||||||
from pandas.compat.numpy import function as nv | ||||||
from pandas.errors import AbstractMethodError | ||||||
|
@@ -152,6 +153,9 @@ class SelectionMixin: | |||||
object sub-classes need to define: obj, exclusions | ||||||
""" | ||||||
|
||||||
obj: Any | ||||||
ivanovmg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
exclusions: Set[Label] = set() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like the docbuild failure is bc this is public but undocumented There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docstring states There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to initialize if sub-classes need to define as stated in the docstring, declaration will silence those errors.
Suggested change
|
||||||
|
||||||
_selection: Optional[IndexLabel] = None | ||||||
_internal_names = ["_cache", "__setstate__"] | ||||||
_internal_names_set = set(_internal_names) | ||||||
|
@@ -206,77 +210,41 @@ def _selection_list(self): | |||||
|
||||||
@cache_readonly | ||||||
def _selected_obj(self): | ||||||
# pandas\core\base.py:195: error: "SelectionMixin" has no attribute | ||||||
# "obj" [attr-defined] | ||||||
if self._selection is None or isinstance( | ||||||
self.obj, ABCSeries # type: ignore[attr-defined] | ||||||
): | ||||||
# pandas\core\base.py:194: error: "SelectionMixin" has no attribute | ||||||
# "obj" [attr-defined] | ||||||
return self.obj # type: ignore[attr-defined] | ||||||
else: | ||||||
# pandas\core\base.py:204: error: "SelectionMixin" has no attribute | ||||||
# "obj" [attr-defined] | ||||||
return self.obj[self._selection] # type: ignore[attr-defined] | ||||||
if self._selection is None or isinstance(self.obj, ABCSeries): | ||||||
return self.obj | ||||||
return self.obj[self._selection] | ||||||
|
||||||
@cache_readonly | ||||||
def ndim(self) -> int: | ||||||
return self._selected_obj.ndim | ||||||
|
||||||
@cache_readonly | ||||||
def _obj_with_exclusions(self): | ||||||
# pandas\core\base.py:209: error: "SelectionMixin" has no attribute | ||||||
# "obj" [attr-defined] | ||||||
if self._selection is not None and isinstance( | ||||||
self.obj, ABCDataFrame # type: ignore[attr-defined] | ||||||
): | ||||||
# pandas\core\base.py:217: error: "SelectionMixin" has no attribute | ||||||
# "obj" [attr-defined] | ||||||
return self.obj.reindex( # type: ignore[attr-defined] | ||||||
columns=self._selection_list | ||||||
) | ||||||
|
||||||
# pandas\core\base.py:207: error: "SelectionMixin" has no attribute | ||||||
# "exclusions" [attr-defined] | ||||||
if len(self.exclusions) > 0: # type: ignore[attr-defined] | ||||||
# pandas\core\base.py:208: error: "SelectionMixin" has no attribute | ||||||
# "obj" [attr-defined] | ||||||
if self._selection is not None and isinstance(self.obj, ABCDataFrame): | ||||||
return self.obj.reindex(columns=self._selection_list) | ||||||
|
||||||
# pandas\core\base.py:208: error: "SelectionMixin" has no attribute | ||||||
# "exclusions" [attr-defined] | ||||||
return self.obj.drop(self.exclusions, axis=1) # type: ignore[attr-defined] | ||||||
if len(self.exclusions) > 0: | ||||||
return self.obj.drop(self.exclusions, axis=1) | ||||||
else: | ||||||
# pandas\core\base.py:210: error: "SelectionMixin" has no attribute | ||||||
# "obj" [attr-defined] | ||||||
return self.obj # type: ignore[attr-defined] | ||||||
return self.obj | ||||||
|
||||||
def __getitem__(self, key): | ||||||
if self._selection is not None: | ||||||
raise IndexError(f"Column(s) {self._selection} already selected") | ||||||
|
||||||
if isinstance(key, (list, tuple, ABCSeries, ABCIndexClass, np.ndarray)): | ||||||
# pandas\core\base.py:217: error: "SelectionMixin" has no attribute | ||||||
# "obj" [attr-defined] | ||||||
if len( | ||||||
self.obj.columns.intersection(key) # type: ignore[attr-defined] | ||||||
) != len(key): | ||||||
# pandas\core\base.py:218: error: "SelectionMixin" has no | ||||||
# attribute "obj" [attr-defined] | ||||||
bad_keys = list( | ||||||
set(key).difference(self.obj.columns) # type: ignore[attr-defined] | ||||||
) | ||||||
if len(self.obj.columns.intersection(key)) != len(key): | ||||||
bad_keys = list(set(key).difference(self.obj.columns)) | ||||||
raise KeyError(f"Columns not found: {str(bad_keys)[1:-1]}") | ||||||
return self._gotitem(list(key), ndim=2) | ||||||
|
||||||
elif not getattr(self, "as_index", False): | ||||||
# error: "SelectionMixin" has no attribute "obj" [attr-defined] | ||||||
if key not in self.obj.columns: # type: ignore[attr-defined] | ||||||
if key not in self.obj.columns: | ||||||
raise KeyError(f"Column not found: {key}") | ||||||
return self._gotitem(key, ndim=2) | ||||||
|
||||||
else: | ||||||
# error: "SelectionMixin" has no attribute "obj" [attr-defined] | ||||||
if key not in self.obj: # type: ignore[attr-defined] | ||||||
if key not in self.obj: | ||||||
raise KeyError(f"Column not found: {key}") | ||||||
return self._gotitem(key, ndim=1) | ||||||
|
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obj
here isAny
andFrameOrSeries
in docstring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks odd.
Initially I typed
obj
asFrameOrSeries
, but then mypy would raise errors of this kind.The reason is that
DataFrame
andSeries
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)