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.

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Removed shadowing of target variable
  • Loading branch information
WillAyd committed Jul 21, 2019
commit 58897155f75a5b93e2918e3f9ba3ed445acae0db
1 change: 1 addition & 0 deletions pandas/core/dtypes/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ 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") # type: ignore
except TypeError:
Expand Down
48 changes: 24 additions & 24 deletions pandas/core/indexes/interval.py
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) # type: ignore
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,21 @@ 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,9 +973,9 @@ def get_indexer_non_unique(
indexer.append(locs)
indexer = np.concatenate(indexer)
else:
target = self._maybe_convert_i8(target)
target_as_index = self._maybe_convert_i8(target_as_index)
indexer, missing = self._engine.get_indexer_non_unique(
target.values # type: ignore
target_as_index.values
)

return ensure_platform_int(indexer), ensure_platform_int(missing)
Expand Down