-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Removed ABCs from pandas._typing #27424
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 all commits
9c6e616
6490263
cce9afb
1fe6059
5f2fa6a
72d3e2c
c3bd6df
5a8d35a
5889715
ed2ee7f
095aed4
4bf254c
8f4a7a1
5e77b75
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 |
---|---|---|
@@ -1,34 +1,29 @@ | ||
from pathlib import Path | ||
from typing import IO, AnyStr, TypeVar, Union | ||
from typing import IO, TYPE_CHECKING, AnyStr, TypeVar, Union | ||
|
||
import numpy as np | ||
|
||
from pandas._libs import Timestamp | ||
from pandas._libs.tslibs.period import Period | ||
from pandas._libs.tslibs.timedeltas import Timedelta | ||
# To prevent import cycles place any internal imports in the branch below | ||
# and use a string literal forward reference to it in subsequent types | ||
# https://mypy.readthedocs.io/en/latest/common_issues.html#import-cycles | ||
if TYPE_CHECKING: | ||
from pandas._libs import Period, Timedelta, Timestamp # noqa: F401 | ||
from pandas.core.arrays.base import ExtensionArray # noqa: F401 | ||
from pandas.core.dtypes.dtypes import ExtensionDtype # noqa: F401 | ||
from pandas.core.indexes.base import Index # noqa: F401 | ||
from pandas.core.frame import DataFrame # noqa: F401 | ||
from pandas.core.series import Series # noqa: F401 | ||
from pandas.core.sparse.series import SparseSeries # noqa: F401 | ||
|
||
from pandas.core.dtypes.dtypes import ExtensionDtype | ||
from pandas.core.dtypes.generic import ( | ||
ABCDataFrame, | ||
ABCExtensionArray, | ||
ABCIndexClass, | ||
ABCSeries, | ||
ABCSparseSeries, | ||
) | ||
|
||
AnyArrayLike = TypeVar( | ||
"AnyArrayLike", | ||
ABCExtensionArray, | ||
ABCIndexClass, | ||
ABCSeries, | ||
ABCSparseSeries, | ||
np.ndarray, | ||
"AnyArrayLike", "ExtensionArray", "Index", "Series", "SparseSeries", np.ndarray | ||
) | ||
ArrayLike = TypeVar("ArrayLike", ABCExtensionArray, np.ndarray) | ||
DatetimeLikeScalar = TypeVar("DatetimeLikeScalar", Period, Timestamp, Timedelta) | ||
Dtype = Union[str, np.dtype, ExtensionDtype] | ||
ArrayLike = TypeVar("ArrayLike", "ExtensionArray", np.ndarray) | ||
DatetimeLikeScalar = TypeVar("DatetimeLikeScalar", "Period", "Timestamp", "Timedelta") | ||
Dtype = Union[str, np.dtype, "ExtensionDtype"] | ||
FilePathOrBuffer = Union[str, Path, IO[AnyStr]] | ||
|
||
FrameOrSeries = TypeVar("FrameOrSeries", ABCSeries, ABCDataFrame) | ||
FrameOrSeries = TypeVar("FrameOrSeries", "Series", "DataFrame") | ||
Scalar = Union[str, int, float] | ||
Axis = Union[str, int] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -906,35 +906,35 @@ def get_indexer( | |
) | ||
raise InvalidIndexError(msg) | ||
|
||
target = ensure_index(target) | ||
target_as_index = ensure_index(target) | ||
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. if you also make 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 thing though - this just loosens the type checking which isn't desired. Actually moved towards TypeVars for reasons described in #26453 (comment) Might update #27050 to include some of that info 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 that applies here.
so reassigning with an 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. We could add Union alternatives for each TypeVar in the central module but I think that confounds the point of generic programming and/or makes our type system weaker. Another option would be to allow redefinition of variables which mypy supplies a setting for: But I also think that makes for a weaker type system, and generally there's not a lot of downside to creating a separate variable here instead of allowing it's type to implicitly be altered by the return of 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.
definitely. we should rule this out.
disagree. the return type of is creating new variables not weakening the type system? |
||
|
||
if isinstance(target, IntervalIndex): | ||
if isinstance(target_as_index, IntervalIndex): | ||
# equal indexes -> 1:1 positional match | ||
if self.equals(target): | ||
if self.equals(target_as_index): | ||
return np.arange(len(self), dtype="intp") | ||
|
||
# different closed or incompatible subtype -> no matches | ||
common_subtype = find_common_type( | ||
[self.dtype.subtype, target.dtype.subtype] | ||
[self.dtype.subtype, target_as_index.dtype.subtype] | ||
) | ||
if self.closed != target.closed or is_object_dtype(common_subtype): | ||
return np.repeat(np.intp(-1), len(target)) | ||
if self.closed != target_as_index.closed or is_object_dtype(common_subtype): | ||
return np.repeat(np.intp(-1), len(target_as_index)) | ||
|
||
# non-overlapping -> at most one match per interval in target | ||
# non-overlapping -> at most one match per interval in target_as_index | ||
# want exact matches -> need both left/right to match, so defer to | ||
# left/right get_indexer, compare elementwise, equality -> match | ||
left_indexer = self.left.get_indexer(target.left) | ||
right_indexer = self.right.get_indexer(target.right) | ||
left_indexer = self.left.get_indexer(target_as_index.left) | ||
right_indexer = self.right.get_indexer(target_as_index.right) | ||
indexer = np.where(left_indexer == right_indexer, left_indexer, -1) | ||
elif not is_object_dtype(target): | ||
elif not is_object_dtype(target_as_index): | ||
# homogeneous scalar index: use IntervalTree | ||
target = self._maybe_convert_i8(target) | ||
indexer = self._engine.get_indexer(target.values) | ||
target_as_index = self._maybe_convert_i8(target_as_index) | ||
indexer = self._engine.get_indexer(target_as_index.values) | ||
else: | ||
# heterogeneous scalar index: defer elementwise to get_loc | ||
# (non-overlapping so get_loc guarantees scalar of KeyError) | ||
indexer = [] | ||
for key in target: | ||
for key in target_as_index: | ||
try: | ||
loc = self.get_loc(key) | ||
except KeyError: | ||
|
@@ -947,21 +947,26 @@ def get_indexer( | |
def get_indexer_non_unique( | ||
self, target: AnyArrayLike | ||
) -> Tuple[np.ndarray, np.ndarray]: | ||
target = ensure_index(target) | ||
target_as_index = ensure_index(target) | ||
|
||
# check that target IntervalIndex is compatible | ||
if isinstance(target, IntervalIndex): | ||
# check that target_as_index IntervalIndex is compatible | ||
if isinstance(target_as_index, IntervalIndex): | ||
common_subtype = find_common_type( | ||
[self.dtype.subtype, target.dtype.subtype] | ||
[self.dtype.subtype, target_as_index.dtype.subtype] | ||
) | ||
if self.closed != target.closed or is_object_dtype(common_subtype): | ||
if self.closed != target_as_index.closed or is_object_dtype(common_subtype): | ||
# different closed or incompatible subtype -> no matches | ||
return np.repeat(-1, len(target)), np.arange(len(target)) | ||
return ( | ||
np.repeat(-1, len(target_as_index)), | ||
np.arange(len(target_as_index)), | ||
) | ||
|
||
if is_object_dtype(target) or isinstance(target, IntervalIndex): | ||
# target might contain intervals: defer elementwise to get_loc | ||
if is_object_dtype(target_as_index) or isinstance( | ||
target_as_index, IntervalIndex | ||
): | ||
# target_as_index might contain intervals: defer elementwise to get_loc | ||
indexer, missing = [], [] | ||
for i, key in enumerate(target): | ||
for i, key in enumerate(target_as_index): | ||
try: | ||
locs = self.get_loc(key) | ||
if isinstance(locs, slice): | ||
|
@@ -973,8 +978,10 @@ def get_indexer_non_unique( | |
indexer.append(locs) | ||
indexer = np.concatenate(indexer) | ||
else: | ||
target = self._maybe_convert_i8(target) | ||
indexer, missing = self._engine.get_indexer_non_unique(target.values) | ||
target_as_index = self._maybe_convert_i8(target_as_index) | ||
indexer, missing = self._engine.get_indexer_non_unique( | ||
target_as_index.values | ||
) | ||
|
||
return ensure_platform_int(indexer), ensure_platform_int(missing) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -240,7 +240,7 @@ def _prep_values(self, values: Optional[np.ndarray] = None) -> np.ndarray: | |
|
||
return values | ||
|
||
def _wrap_result(self, result, block=None, obj=None) -> FrameOrSeries: | ||
def _wrap_result(self, result, block=None, obj=None): | ||
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. Not ideal to remove Options discussed were:
I looked at option 1 first it caused a lot of flake8 complaints with other imports in the module where we import Series directly within a function, so would add a lot of noqa directives to the file to make it work. Option 2 was suggested by @simonjayhawkins and would be viable but I figured just remove for now and can open up a follow up issue to try and make this function generic. It was type checking to 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.
adding having so I don't think any PRs adding type annotations should be held pending perfection (and especially unrelated PRs with "Any interest in adding an annotation while you are looking at this?" . xref #26795) 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. Just so we are on the same page anything without an annotation is implicitly How do you view the 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.
+1 for this. I strongly prefer to leave something un-typed as an invitation to the next person to try to figure it out. 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.
just done an experiment.. it appears that Any is considered a type annotation and so the code gets checked, whereas leaving it out causes the type checker to not check the code. so -1 for not adding Any to un-typed function signatures. 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. What's a case where having a function typed with Any everywhere is valuable? 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.
not everywhere! just where they are currently omitted because the union is of too many types. such as key in indexing. 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.
Have to double check but it's not the field you were actually annotated that throws the error as much as something else right? I think mypy might skip a function if it has no annotations so adding Any may cause it to throw errors for inferences of local variables it can't make. With that said I really don't think adding This thread is probably best as a separate issue 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.
that's correct. or even incorrect calls to typed functions.
disagree.
ok locally for a module, but how to set-up on the CI? if we are typing gradually we are not yet ready for this. Adding Any to a function, indicates the body of that function is ready to be type checked.
i'm opening another PR shortly, where Any is added for all unions of 5 or more items. https://monkeytype.readthedocs.io/en/latest/generation.html#monkeytype.typing.RewriteLargeUnion so we can continue the discusion there. |
||
""" | ||
Wrap a single result. | ||
""" | ||
|
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.
quote from https://mypy.readthedocs.io/en/latest/generics.html...
"User-defined generics are a moderately advanced feature and you can get far without ever using them..."
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.
Thanks but this just loosens the type system rather than actually fixing anything. TypeVar is going to be generally more useful for checking functions that can be fully generic in nature.
Might just change the return of this one and see how many others require Union in the future
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.
makes sense. Union[Series, DataFrame] might be better written as NDFrame anyway?
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.
Also the "user-defined generics" you are referring to are more applicable to containers not TypeVars. Right now we just use a blanket
Series
as a return object, though in the future we could do something likeSeries[int]
andSeries[str]
, etc...; theSeries
would be the user-defined generic in that caseThe TypeVar in the docs you linked is just a way of parametrizing that user-defined generic, so that a
Series[int]
would have to stay as aSeries[int]
through it's lifecycle; without that parametrization we allowSeries[int]
to becomeSeries[str]
without any complaints from mypy todayWe are probably a ways off of doing user-defined generics but this is great that you looked into it. Certainly open to ideas on that front if you think of a good way to implement as we get more familiar with these annotations
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.
Hmm that would work though we don't typically import NDFrame anywhere so I don't think want to start here
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.
I would leave as FrameOrSeries as its more descriptive