TYP: Add cast to ABC classes. #37902

rhshadrach · 2020-11-16T23:00:05Z

Implements the idea of @simonjayhawkins from #27353 (comment) to use cast on ABC classes. Currently only implements ABCDataFrame, ABCNDFrame, and ABCSeries. Previously, mypy would resolve e.g. ABCSeries as any, now it gets resolved as Series.

One issue remains: I don't understand why the code below generates this complaint.

pandas/core/common.py:472: error: Incompatible return value type (got "Union[Any, List[Any], Series]", expected "Union[List[Any], ExtensionArray]") [return-value]

def convert_to_list_like(
    values: Union[Scalar, Iterable, AnyArrayLike]
) -> Union[List, AnyArrayLike]:
    if isinstance(values, (list, np.ndarray, ABCIndex, ABCSeries, ABCExtensionArray)):
        return values
    ...

One can add Series to the return Union to resolve this, but it doesn't seem like that should be necessary.

I'm also getting:

pandas/core/base.py:923: note: Internal mypy error checking function redefinition

on the lines

def map_f(values, f):
    return lib.map_infer_mask(values, f, isna(values).view(np.uint8))

where map_f is defined in various ways in if-else blocks. I'm not sure if this is a mypy bug.

pandas/core/algorithms.py

pandas/core/computation/parsing.py

jbrockmendel · 2020-11-16T23:06:24Z

pandas/core/construction.py

-def extract_array(obj: AnyArrayLike, extract_numpy: bool = False) -> ArrayLike:
+def extract_array(
+    obj: AnyArrayLike, extract_numpy: bool = False
+) -> Union[AnyArrayLike, ExtensionArray, np.ndarray]:


i dont think we want AnyArrayLike in the return type, since we should never return Series or Index

we will probably need to use an overload here eventually but not so easy when np.ndarray resolves to Any as we normally get messages about overlapping overloads.

Suggested change

) -> Union[AnyArrayLike, ExtensionArray, np.ndarray]:

) -> Union[Any, ExtensionArray, np.ndarray]:

for now.

Thanks, looking at this more closely, the call to array always returns an EA. The possible types for input are EA, Index, Series, or np.ndarray, so that means the return can only be EA or np.ndarray. Let me know if this wrong, for now I've simply removed the AnyArrayLike.

pandas/core/computation/align.py

pandas/core/computation/parsing.py

pandas/core/dtypes/generic.py

pandas/core/reshape/concat.py

pandas/core/window/common.py

simonjayhawkins · 2020-11-17T15:10:23Z

pandas/core/construction.py

-def extract_array(obj: AnyArrayLike, extract_numpy: bool = False) -> ArrayLike:
+def extract_array(
+    obj: AnyArrayLike, extract_numpy: bool = False
+) -> Union[AnyArrayLike, ExtensionArray, np.ndarray]:


we will probably need to use an overload here eventually but not so easy when np.ndarray resolves to Any as we normally get messages about overlapping overloads.

Suggested change

) -> Union[AnyArrayLike, ExtensionArray, np.ndarray]:

) -> Union[Any, ExtensionArray, np.ndarray]:

for now.

simonjayhawkins · 2020-11-17T15:20:40Z

pandas/core/construction.py

+        result = obj.array
+    else:
+        result = obj

-    if extract_numpy and isinstance(obj, ABCPandasArray):
-        obj = obj.to_numpy()
+    if extract_numpy and isinstance(result, ABCPandasArray):
+        result = result.to_numpy()

-    # error: Incompatible return value type (got "Index", expected "ExtensionArray")
-    # error: Incompatible return value type (got "Series", expected "ExtensionArray")
-    return obj  # type: ignore[return-value]
+    return result


the problem here is that AnyArrayLike is a typevar and also the type of obj is wrong anyway and should be object I think could also change obj to object/Any for now until we overload and keep the implementation unchanged.

can you revert the changes to the implementation.

Yes, but would require adding obj = obj.array # type: ignore[assignment] or possibly changing typing on arguments (not return type)

IIUC this is a similar issue to #37902 (comment) where we changed FrameOrSeries to FrameOrSeriesUnion.

We don't yet have an AnyArrayLikeUnion to do the same here.

because AnyArrayLike is a typevar, the type annotations imply that the type doesn't change.

This is not true. Only for np.ndarray and ExtensionArray if extract_array=False.

so maybe the signature should be

def extract_array( obj: Union[object, ArrayLike], extract_numpy: bool = False ) -> Union[object, ArrayLike]:

but we also need to overload (to account for Series and Index array extraction) but while numpy types resolve to Any we get mypy errors about overlapping overloads.

so for now, i think ok to do...

def extract_array( obj: object, extract_numpy: bool = False ) -> Union[Any, ArrayLike]:

It's the use of the typevar that causes the issue, so I'm not too concerned what type annotations are chosen at this stage, so long as we are not changing the implementation because we are using a TypeVar incorrectly.

Thanks for the explanation! Changes made.

simonjayhawkins · 2020-11-17T15:33:57Z

One issue remains: I don't understand why the code below generates this complaint.

pandas/core/common.py:472: error: Incompatible return value type (got "Union[Any, List[Any], Series]", expected "Union[List[Any], ExtensionArray]") [return-value]
def convert_to_list_like(
    values: Union[Scalar, Iterable, AnyArrayLike]
) -> Union[List, AnyArrayLike]:
    if isinstance(values, (list, np.ndarray, ABCIndex, ABCSeries, ABCExtensionArray)):
        return values
    ...
One can add Series to the return Union to resolve this, but it doesn't seem like that should be necessary.

AnyArrayLike is a TypeVar so that a type of AnyArrayLike is returned as is. I'm guessing this is a False positive that may be down to either np.ndarray resolving to Any or down to ABCIndex and ABCExtensionArray not yet updated to not resolve to Any. I would just ignore these errors for now.

simonjayhawkins · 2020-11-17T16:00:11Z

I'm also getting:

pandas/core/base.py:923: note: Internal mypy error checking function redefinition

on the lines
def map_f(values, f):
    return lib.map_infer_mask(values, f, isna(values).view(np.uint8))
where map_f is defined in various ways in if-else blocks. I'm not sure if this is a mypy bug.

changing the def to a lambda fixes. probably a mypy issue.

rhshadrach · 2020-11-18T01:57:03Z

Thanks for all the feedback @jbrockmendel and @simonjayhawkins; I believe I've address everything thus far.

simonjayhawkins · 2020-11-18T11:41:55Z

pandas/plotting/_matplotlib/tools.py


-    cellText = data.values
+    cellText = frame.values


the changes in this module are not required. The mypy issue is trying to assign the result of data._to_frame back to data which is FrameOrSeries, a typevar.

just need to change FrameOrSeries to FrameOrSeriesUnion in the function signature.

I think the defaults in pandas._typing should be changed to Unions. see also #36092 (comment) about ArrayLike and AnyArrayLike

…al error

rhshadrach · 2020-11-18T22:43:53Z

Thanks @simonjayhawkins, change made. I also used your lambda to work around the internal mypy error for a lack of any other solutions. Using # type: ignore worked to silence mypy, but there is no error type and so it doesn't pass the pre-commit style checks. On a whim I attempted # type: ignore[note] but it did not work. I'll be raising an issue with mypy later tonight.

simonjayhawkins · 2020-11-22T13:24:35Z

Using # type: ignore worked to silence mypy, but there is no error type and so it doesn't pass the pre-commit style checks. On a whim I attempted # type: ignore[note] but it did not work. I'll be raising an issue with mypy later tonight.

we could perhaps change our code check to allow, say, # noqa to be added to allow this through.

simonjayhawkins

Nice. Thanks @rhshadrach for working on this. generally lgtm. looking forward to the other ABC getting the same treatment.

simonjayhawkins · 2020-11-22T13:28:02Z

pandas/core/construction.py

+        result = obj.array
+    else:
+        result = obj

-    if extract_numpy and isinstance(obj, ABCPandasArray):
-        obj = obj.to_numpy()
+    if extract_numpy and isinstance(result, ABCPandasArray):
+        result = result.to_numpy()

-    # error: Incompatible return value type (got "Index", expected "ExtensionArray")
-    # error: Incompatible return value type (got "Series", expected "ExtensionArray")
-    return obj  # type: ignore[return-value]
+    return result


can you revert the changes to the implementation.

pandas/core/computation/parsing.py

simonjayhawkins

Thanks @rhshadrach lgtm.

jreback

looks fine, happy to handle in a followup or here. ping either way

pandas/core/computation/align.py

jreback · 2020-11-23T13:50:19Z

pandas/core/computation/parsing.py

@@ -102,12 +104,12 @@ def clean_column_name(name: str) -> str:

    Parameters
    ----------
-    name : str
+    name : hashable


shouldn't this be Label (or is this consistent with how we do elsewhere?)

A quick search reveals 6 counts of "Label", 2 counts of "hashable" (other than the 2 added in this PR). @simonjayhawkins requested hashable over Label, and I like the idea of staying away from pandas-defined types for readability. This is an internal function, so that reasoning doesn't directly apply, but it seems best to me to not have a conditional when deciding on typing in docstrings (e.g. hashable when function is public, Label otherwise).

kk that's fine, just worried that we are inconsistent generally. i am not sure this is actually easy to check, but we should try to be as consisten as possible.

pandas/core/dtypes/generic.py

pandas/core/internals/construction.py

rhshadrach · 2020-11-23T18:04:52Z

@jreback changes made and green. Just the label vs hashable issue is outstanding.

jreback · 2020-11-24T13:30:33Z

thanks @rhshadrach

TYP: Add cast to ABC classes.

8a466ac

rhshadrach added the Typing type annotations, mypy/pyright type checking label Nov 16, 2020

jbrockmendel reviewed Nov 16, 2020

View reviewed changes

pandas/core/algorithms.py Show resolved Hide resolved

jbrockmendel reviewed Nov 16, 2020

View reviewed changes

pandas/core/computation/parsing.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Nov 16, 2020

View reviewed changes

simonjayhawkins reviewed Nov 17, 2020

View reviewed changes

pandas/core/computation/align.py Outdated Show resolved Hide resolved

simonjayhawkins reviewed Nov 17, 2020

View reviewed changes

pandas/core/computation/parsing.py Show resolved Hide resolved

pandas/core/dtypes/generic.py Outdated Show resolved Hide resolved

pandas/core/reshape/concat.py Outdated Show resolved Hide resolved

simonjayhawkins reviewed Nov 17, 2020

View reviewed changes

Feedback fixes

99c9e6b

simonjayhawkins reviewed Nov 18, 2020

View reviewed changes

Revert changes using FrameOrSeriesUnion; lambda to work around intern…

f8a76bc

…al error

simonjayhawkins reviewed Nov 22, 2020

View reviewed changes

rhshadrach added 2 commits November 22, 2020 12:44

Implementation revert and docstring type

c9a31a4

Fix extract_array typing

1ac7c9e

simonjayhawkins approved these changes Nov 23, 2020

View reviewed changes

simonjayhawkins added this to the 1.2 milestone Nov 23, 2020

jreback approved these changes Nov 23, 2020

View reviewed changes

rhshadrach added 2 commits November 23, 2020 10:16

future annotations, better typing of indexes

2ba5fbe

Revert quote removal from pandas.core.generic

bcd0257

jreback merged commit 99376b3 into pandas-dev:master Nov 24, 2020

rhshadrach deleted the abc_typing branch November 24, 2020 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TYP: Add cast to ABC classes. #37902

TYP: Add cast to ABC classes. #37902

rhshadrach commented Nov 16, 2020 •

edited

Loading

jbrockmendel Nov 16, 2020

simonjayhawkins Nov 17, 2020

rhshadrach Nov 18, 2020

simonjayhawkins Nov 17, 2020

simonjayhawkins Nov 17, 2020

simonjayhawkins Nov 22, 2020

rhshadrach Nov 22, 2020 •

edited

Loading

simonjayhawkins Nov 22, 2020

rhshadrach Nov 22, 2020

simonjayhawkins commented Nov 17, 2020

simonjayhawkins commented Nov 17, 2020

rhshadrach commented Nov 18, 2020

simonjayhawkins Nov 18, 2020

rhshadrach commented Nov 18, 2020

simonjayhawkins commented Nov 22, 2020

simonjayhawkins left a comment

simonjayhawkins Nov 22, 2020

simonjayhawkins left a comment

jreback left a comment

jreback Nov 23, 2020

rhshadrach Nov 23, 2020

jreback Nov 24, 2020

rhshadrach commented Nov 23, 2020

jreback commented Nov 24, 2020

	) -> Union[AnyArrayLike, ExtensionArray, np.ndarray]:
	) -> Union[Any, ExtensionArray, np.ndarray]:

TYP: Add cast to ABC classes. #37902

TYP: Add cast to ABC classes. #37902

Conversation

rhshadrach commented Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhshadrach Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins commented Nov 17, 2020

simonjayhawkins commented Nov 17, 2020

rhshadrach commented Nov 18, 2020

Choose a reason for hiding this comment

rhshadrach commented Nov 18, 2020

simonjayhawkins commented Nov 22, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhshadrach commented Nov 23, 2020

jreback commented Nov 24, 2020

rhshadrach commented Nov 16, 2020 •

edited

Loading

rhshadrach Nov 22, 2020 •

edited

Loading