Skip to content

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.

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

Merged
merged 14 commits into from
Jul 24, 2019
Merged

Removed ABCs from pandas._typing #27424

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9c6e616
Removed ABCs from pandas._typing
WillAyd Jul 16, 2019
6490263
Fix CI
WillAyd Jul 16, 2019
cce9afb
Fixed type ignore location
WillAyd Jul 16, 2019
1fe6059
Merge remote-tracking branch 'upstream/master' into remove-abcs
WillAyd Jul 17, 2019
5f2fa6a
Excluded from coveraged and updated tslib imports
WillAyd Jul 17, 2019
72d3e2c
Merge remote-tracking branch 'upstream/master' into remove-abcs
WillAyd Jul 21, 2019
c3bd6df
Merge branch 'remove-abcs' of github.com:WillAyd/pandas into remove-abcs
WillAyd Jul 21, 2019
5a8d35a
Comments in typing
WillAyd Jul 21, 2019
5889715
Removed shadowing of target variable
WillAyd Jul 21, 2019
ed2ee7f
Blackify
WillAyd Jul 21, 2019
095aed4
Removed failing annotation
WillAyd Jul 22, 2019
4bf254c
Removed unused imports
WillAyd Jul 22, 2019
8f4a7a1
Merge remote-tracking branch 'upstream/master' into remove-abcs
WillAyd Jul 23, 2019
5e77b75
Merge remote-tracking branch 'upstream/master' into remove-abcs
WillAyd Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 25 additions & 22 deletions pandas/_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,37 @@
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

from pandas.core.arrays.base import ExtensionArray
from pandas.core.dtypes.dtypes import ExtensionDtype
from pandas.core.dtypes.generic import (
ABCDataFrame,
ABCExtensionArray,
ABCIndexClass,
ABCSeries,
ABCSparseSeries,
)
from pandas.core.indexes.base import Index
from pandas.core.frame import DataFrame
from pandas.core.series import Series
from pandas.core.sparse.series import SparseSeries

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")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FrameOrSeries = TypeVar("FrameOrSeries", "Series", "DataFrame")
FrameOrSeries = Union["Series", "DataFrame"]

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..."

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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 like Series[int] and Series[str], etc...; the Series would be the user-defined generic in that case

The 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 a Series[int] through it's lifecycle; without that parametrization we allow Series[int] to become Series[str] without any complaints from mypy today

We 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

Copy link
Member Author

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?

Hmm that would work though we don't typically import NDFrame anywhere so I don't think want to start here

Copy link
Contributor

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

Scalar = Union[str, int, float]
Axis = Union[str, int]
5 changes: 3 additions & 2 deletions pandas/core/dtypes/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,13 @@ def ensure_int_or_float(arr: ArrayLike, copy=False) -> np.array:
If the array is explicitly of type uint64 the type
will remain unchanged.
"""
# TODO: GH27506 potential bug with ExtensionArrays
try:
return arr.astype("int64", copy=copy, casting="safe")
return arr.astype("int64", copy=copy, casting="safe") # type: ignore
except TypeError:
pass
try:
return arr.astype("uint64", copy=copy, casting="safe")
return arr.astype("uint64", copy=copy, casting="safe") # type: ignore
except TypeError:
return arr.astype("float64", copy=copy)

Expand Down
55 changes: 31 additions & 24 deletions pandas/core/indexes/interval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,35 +906,35 @@ def get_indexer(
)
raise InvalidIndexError(msg)

target = ensure_index(target)
target_as_index = ensure_index(target)
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 also make AnyArrayLike an Alias instead of a TypeVar, then this module doesn't need to be changed at all.

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

Actually moved towards TypeVars for reasons described in #26453 (comment)

i don't think that applies here.

target does not need to be a TypeVar, the type of target does not need to be maintained.

target can be any of ["ExtensionArray", "Index", "Series", "SparseSeries", np.ndarray]

ensure_index is not yet typed. but presumably will have a return type of Index.

so reassigning with an Index to a variable that has a type of Union of ["ExtensionArray", "Index", "Series", "SparseSeries", np.ndarray] is perfectly valid.

Copy link
Member Author

Choose a reason for hiding this comment

The 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:

https://mypy.readthedocs.io/en/latest/command_line.html?highlight=allow-redefinition#miscellaneous-strictness-flags

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 ensure_index

Copy link
Member

Choose a reason for hiding this comment

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

Another option would be to allow redefinition of variables which mypy supplies a setting for:

https://mypy.readthedocs.io/en/latest/command_line.html?highlight=allow-redefinition#miscellaneous-strictness-flags

But I also think that makes for a weaker type system

definitely. we should rule this out.

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 ensure_index

disagree. the return type of ensure_index would need to conform to the type of target.

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:
Expand All @@ -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):
Expand All @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion pandas/core/window.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member Author

Choose a reason for hiding this comment

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

Not ideal to remove FrameOrSeries here but mypy was complaining since this function is not fully generic (i.e. a conditional branch could determine the returned object)

Options discussed were:

  1. Use Union["Series, "DataFrame"] as a return type
  2. Use NDFrame as a return type
  3. Remove

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 Any before this change anyway so we aren't loosing any inference. Open to thoughts

Copy link
Member

Choose a reason for hiding this comment

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

It was type checking to Any before this change anyway so we aren't loosing any inference.

adding Any (or not typing) is not so bad. There is a network effect benefit from typing, so the quicker we add type hints to modules, the quicker we see a benefit.

having Any can easily be picked up in a future pass with one of many cli flags to mypy.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The 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 Any so typing it as such doesn't change anything.

How do you view the Any annotations currently in the code? I interpret that as "this has already been reviewed and really should allow anything" versus "I just added Any to get this to pass". I don't see a lot of value add to the latter since that's what happens implicitly without an annotation anyway but certainly a point for discussion

Copy link
Member

Choose a reason for hiding this comment

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

I interpret that as "this has already been reviewed and really should allow anything" versus "I just added Any to get this to pass". I don't see a lot of value add to the latter since that's what happens implicitly without an annotation

+1 for this. I strongly prefer to leave something un-typed as an invitation to the next person to try to figure it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

Copy link
Member

Choose a reason for hiding this comment

The 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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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?

not everywhere!

just where they are currently omitted because the union is of too many types. such as key in indexing.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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 Any is a great approach. If we wanted the former there is a --check-untyped-def flag we could enable.

This thread is probably best as a separate issue

Copy link
Member

Choose a reason for hiding this comment

The 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.

that's correct. or even incorrect calls to typed functions.

With that said I really don't think adding Any is a great approach.

disagree.

If we wanted the former there is a --check-untyped-def flag we could enable.

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.

This thread is probably best as a separate issue

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.
"""
Expand Down
5 changes: 4 additions & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ exclude =
.eggs/*.py,
versioneer.py,
env # exclude asv benchmark environments from linting
pandas/_typing.py

[flake8-rst]
bootstrap =
Expand Down Expand Up @@ -77,7 +78,9 @@ filterwarnings =

[coverage:run]
branch = False
omit = */tests/*
omit =
*/tests/*
pandas/_typing.py
plugins = Cython.Coverage

[coverage:report]
Expand Down