-
-
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 15 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 | ||||
---|---|---|---|---|---|---|
|
@@ -10,7 +10,9 @@ | |||||
Callable, | ||||||
Dict, | ||||||
FrozenSet, | ||||||
Hashable, | ||||||
Optional, | ||||||
Set, | ||||||
TypeVar, | ||||||
Union, | ||||||
cast, | ||||||
|
@@ -148,10 +150,20 @@ class SpecificationError(Exception): | |||||
|
||||||
class SelectionMixin: | ||||||
""" | ||||||
mixin implementing the selection & aggregation interface on a group-like | ||||||
object sub-classes need to define: obj, exclusions | ||||||
Mixin for the selection & aggregation interface on a group-like object. | ||||||
|
||||||
Sub-classes need to define: obj, exclusions | ||||||
""" | ||||||
|
||||||
obj: Any | ||||||
ivanovmg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
"""Target object for the selection and aggregation.""" | ||||||
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. remove or change to comment 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. Removed |
||||||
# GH 38239 | ||||||
# TODO obj here must be typed as FrameOrSeriesUnion, | ||||||
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.
Suggested change
|
||||||
# however this creates multiple mypy errors elsewhere. | ||||||
# Those have to be addressed in a separate PR. | ||||||
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. I don't think this adds value. can remove 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. Removed |
||||||
exclusions: Set[Hashable] | ||||||
"""Columns to exclude.""" | ||||||
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. same 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. Removed |
||||||
|
||||||
_selection: Optional[IndexLabel] = None | ||||||
_internal_names = ["_cache", "__setstate__"] | ||||||
_internal_names_set = set(_internal_names) | ||||||
|
@@ -206,77 +218,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 | ||||||
) | ||||||
if self._selection is not None and isinstance(self.obj, ABCDataFrame): | ||||||
return self.obj.reindex(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] | ||||||
|
||||||
# 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, ABCIndex, 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)