TYP: Add types to top-level funcs, step 2 #30582

topper-123 · 2019-12-31T13:32:48Z

Next step to #30565.

Next up will be the pd.read_* functions.

topper-123 · 2019-12-31T13:34:29Z

pandas/core/algorithms.py

@@ -651,7 +654,7 @@ def value_counts(
    normalize: bool = False,
    bins=None,
    dropna: bool = True,
-) -> ABCSeries:
+) -> "Series":


ABCSeries doesn't work as a return type, AFAIS: Doing a x = pd.value_counts(x); reveal_type(x) returns Any, if the return type is ABCSeries.

yes, all ABCs used for type annotations need to be replaced. we need a custom script/clever regex to avoid this in future.

topper-123 · 2019-12-31T13:36:12Z

pandas/core/reshape/reshape.py

-        result = concat(with_dummies, axis=1)
+        concatted = concat(with_dummies, axis=1)
+        assert isinstance(concatted, DataFrame)
+        result = concatted


This is the same annoying mypy issue as in #30565. don't know if there's a better way to do this

simonjayhawkins

Thanks @topper-123

simonjayhawkins · 2019-12-31T13:37:01Z

pandas/core/reshape/concat.py

@@ -37,7 +37,7 @@ def concat(
    verify_integrity: bool = False,
    sort: bool = False,
    copy: bool = True,
-):
+) -> Union["DataFrame", "Series"]:


I assume FrameOrSeries doesn't apply here?

No, the problem is the return type doesn't always follow a parameter type. Specifically if the input are series, and axis=1, the output is a DataFrame. We need Literals to do it better.

simonjayhawkins · 2019-12-31T13:38:02Z

pandas/core/reshape/pivot.py

@@ -58,7 +58,9 @@ def pivot_table(
            pieces.append(table)
            keys.append(getattr(func, "__name__", func))

-        return concat(pieces, keys=keys, axis=1)
+        result = concat(pieces, keys=keys, axis=1)


can concat be overloaded to avoid this?

even without Literal for axis could try List[DataFrame] -> DataFrame and List[Series} -> Union[Series, DataFrame]

Yeah, that might work. I'll look into it.

pep8speaks · 2019-12-31T14:32:26Z

Hello @topper-123! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-05 12:20:24 UTC

topper-123 · 2019-12-31T14:37:25Z

I've overloaded concat. mypy demands at least two overloads, which is strange.

This is quite ugly, but allows us to drop the uglyness in other locations, so...:unamused:

simonjayhawkins · 2019-12-31T14:40:13Z

I've overloaded concat. mypy demands at least two overloads, which is quite strange.

This is quite ugly, but allows us to drop the uglyness in other locations, so...😒

the overloads do end up being ugly (and introduces duplication) but is more benefical for users of types since they don't need to include #ignores, casts or workarounds.

topper-123 · 2019-12-31T14:46:22Z

Yeah, I agree. It can be cleaned a bit when we reach 3.8. hopefully mypy will drop the duplication requirement.

The overload example in the python docs had no empty lines between overloaded definitions. Our Flake8 accepts that with a noqa directive, but pep8speaks doesn't. This is probaly not settled stylewise, but I like having no empty lines to signal the definitions belong together.

EDIT: Ok, while flake8 acceps no empty lines, black doesn't. So two empty lines it will be....

simonjayhawkins · 2019-12-31T15:44:12Z

xref #26766

simonjayhawkins · 2019-12-31T15:53:44Z

pandas/core/reshape/concat.py

 def concat(
-    objs,
+    objs: Union[Sequence[FrameOrSeriesUnion], Mapping[str, FrameOrSeriesUnion]],


i think the mapping key should be Optional[Hashable]

simonjayhawkins · 2019-12-31T15:55:31Z

pandas/core/reshape/concat.py

@@ -25,9 +25,28 @@
 # ---------------------------------------------------------------------
 # Concatenate DataFrame objects

+FrameOrSeriesUnion = Union["DataFrame", "Series"]


maybe we should put this in _typing as FrameOrSeries and rename FrameOrSeries as FrameOrSeriesT. @WillAyd

WillAyd · 2019-12-31T16:29:30Z

i think that’s a great idea

…

Sent from my iPhone

On Dec 31, 2019, at 10:55 AM, Simon Hawkins ***@***.***> wrote: @simonjayhawkins commented on this pull request. In pandas/core/reshape/concat.py: > @@ -25,9 +25,28 @@ # --------------------------------------------------------------------- # Concatenate DataFrame objects +FrameOrSeriesUnion = Union["DataFrame", "Series"] maybe we should put this in _typing as FrameOrSeries and rename FrameOrSeries as FrameOrSeriesT. @WillAyd — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

topper-123 · 2019-12-31T19:30:43Z

I’ve made the changes.

jbrockmendel · 2019-12-31T20:13:20Z

pandas/_typing.py

@@ -41,7 +42,8 @@

 Dtype = Union[str, np.dtype, "ExtensionDtype"]
 FilePathOrBuffer = Union[str, Path, IO[AnyStr]]
-FrameOrSeries = TypeVar("FrameOrSeries", bound="NDFrame")
+FrameOrSeries = Union["DataFrame", "Series"]
+FrameOrSeriesT = TypeVar("FrameOrSeriesT", bound="NDFrame")


can you add a comment explaining the difference between FrameOrSeries vs FrameOrSeriesT

Hmm after viewing the diff here I am second guess this a bit - is this really just for one-off things like concat? If so probably worth just dealing with exceptions in the individual signatures

I don't have a strong opinion on this myself. I can remove it here and if we want this union in _typing.py later, it can be added then?

I would be more descriptive on the name and add a comment before these

Maybe SameFrameOrSeries? It's difficult to find a descriptive name for the TypeVar that isn't very long. It will be used in a lot of places, so shouldn't be too long IMO.

sure that sounds better, definitely add some comments / sections here in typing with liberal explanations on when to use these

I find SameFrameOrSeries is ambiguous - is that supposed to represent the type or instance of the object being the same? Should be type as far as annotations are concerned but I don't think that's clear

I would be +1 with @simonjayhawkins naming suggestion as I think the T is a pseudo-convention seen most with TypeVars in typeshed

jreback

looks good, but I think need to comment / explain when FrameOrSeries and FrameOrSeriesT (need a better name here as well), should be used. would comment in pandas._typing directly (generally we should have many more comments on what types of things these should cover, even if they are 'obvious'.

jreback · 2020-01-01T01:36:07Z

pandas/_typing.py

@@ -41,7 +42,8 @@

 Dtype = Union[str, np.dtype, "ExtensionDtype"]
 FilePathOrBuffer = Union[str, Path, IO[AnyStr]]
-FrameOrSeries = TypeVar("FrameOrSeries", bound="NDFrame")
+FrameOrSeries = Union["DataFrame", "Series"]
+FrameOrSeriesT = TypeVar("FrameOrSeriesT", bound="NDFrame")


I would be more descriptive on the name and add a comment before these

simonjayhawkins · 2020-01-01T08:46:41Z

pandas/core/reshape/concat.py

@@ -26,8 +28,25 @@
 # Concatenate DataFrame objects


+@overload
+def concat(
+    objs: Union[Sequence["DataFrame"], Mapping[Hashable, "DataFrame"]],


IIUC None is a valid key and None is not a subtype of Hashable.

Suggested change

objs: Union[Sequence["DataFrame"], Mapping[Hashable, "DataFrame"]],

objs: Union[Sequence["DataFrame"], Mapping[Optional[Hashable], "DataFrame"]],

None is an instance of Hashable:

In [1]: from typing import Hashable In [2]: isinstance(None, Hashable) Out[2]: True

So Optional is not needed.

however, when typechecking

from typing import Hashable a: Hashable = None

$ mypy test.py test.py:3: error: Incompatible types in assignment (expression has type "None", variable has type "Hashable") Found 1 error in 1 file (checked 1 source file)

Hmm, but None is hashable (e.g. hash(None) and {None: 1} both work). So mypy is specialcasing None, which seems a bit like a bug to me.

Maybe we should have a Label = Optional[Hashable] in _typing.py? Would seem easier to remember to use that than to define Optional[Hashable] every time?

I've added Optional. I did not add a Label type to _typing.py. Figured it can be done in seperate PR by searching for "Optional[Hashable]"

topper-123 · 2020-01-01T21:45:48Z

PR updated.

jreback

minor comment. @simonjayhawkins @WillAyd

pandas/_typing.py

WillAyd · 2020-01-02T14:55:56Z

I think @simonjayhawkins and I are proposing FrameOrSeriesT to denote a TypeVar version and go with a pseudo-standard of appending T for any TypeVars. Currently this is at SameFrameOrSeries which I am personally against as it is ambiguous what "Same" means

Is there a strong objection to the T standard?

topper-123 · 2020-01-02T16:12:21Z

I don’t have a strong opinion either way, but I think ‘FrameOrSeries’ and ‘FrameOrSeriesT’ are too similar; it will be unnecessary difficult to glance at the types. Maybe ‘NDFrameT’ (signals better where it actually comes from)? Or a differrent name for the union type.

simonjayhawkins · 2020-01-02T16:19:04Z

Maybe ‘NDFrameT’ (signals better where it actually comes from)?

prefer not. we may want to change this. see #29480

topper-123 · 2020-01-02T16:51:20Z

FrameOrSeries for the TypeVar and FrameOrSeriesUnion for the union? The union should be used sparingly, so less of a problem if that has a long name.

jreback · 2020-01-03T02:47:29Z

FrameOrSeries for the TypeVar and FrameOrSeriesUnion for the union? The union should be used sparingly, so less of a problem if that has a long name.

sure

pandas/core/reshape/concat.py

topper-123 · 2020-01-05T12:22:39Z

Is this ok to take in?

topper-123 · 2020-01-05T12:51:22Z

The failure looks unrelated, looks cython-related.

jreback · 2020-01-05T16:21:10Z

thanks !

topper-123 commented Dec 31, 2019

View reviewed changes

simonjayhawkins reviewed Dec 31, 2019

View reviewed changes

simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Dec 31, 2019

simonjayhawkins added this to the 1.0 milestone Dec 31, 2019

topper-123 force-pushed the type_top_level_funcs_II branch from 88f9556 to 03375c6 Compare December 31, 2019 15:03

simonjayhawkins reviewed Dec 31, 2019

View reviewed changes

topper-123 force-pushed the type_top_level_funcs_II branch 2 times, most recently from ccba86c to d68fe49 Compare December 31, 2019 18:29

jbrockmendel reviewed Dec 31, 2019

View reviewed changes

jreback requested changes Jan 1, 2020

View reviewed changes

simonjayhawkins reviewed Jan 1, 2020

View reviewed changes

topper-123 force-pushed the type_top_level_funcs_II branch from d68fe49 to 87a9b1a Compare January 1, 2020 21:43

jreback approved these changes Jan 2, 2020

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

topper-123 force-pushed the type_top_level_funcs_II branch from 87a9b1a to ddcdb8a Compare January 2, 2020 02:55

topper-123 force-pushed the type_top_level_funcs_II branch from ddcdb8a to 4270e57 Compare January 3, 2020 08:11

topper-123 force-pushed the type_top_level_funcs_II branch from 4270e57 to c32eef4 Compare January 3, 2020 08:19

jreback requested changes Jan 3, 2020

View reviewed changes

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

topper-123 added 6 commits January 5, 2020 12:15

Add types to more top-level funcs

3ee69ba

overload concat function

b8370e3

Add _typing.FrameOrSeries

0b935ce

Explain typed etc.

2926047

linebreaks

0d49daa

SameFrameOrSeries -> FrameOrSeries

84d1c37

topper-123 force-pushed the type_top_level_funcs_II branch from c32eef4 to 84d1c37 Compare January 5, 2020 12:20

jreback merged commit 66b4dbc into pandas-dev:master Jan 5, 2020

topper-123 deleted the type_top_level_funcs_II branch January 5, 2020 16:27

simonjayhawkins mentioned this pull request Sep 18, 2020

BUG: Concat typing #36409

Merged

5 tasks

	objs: Union[Sequence["DataFrame"], Mapping[Hashable, "DataFrame"]],
	objs: Union[Sequence["DataFrame"], Mapping[Optional[Hashable], "DataFrame"]],

TYP: Add types to top-level funcs, step 2 #30582

TYP: Add types to top-level funcs, step 2 #30582

Conversation

topper-123 commented Dec 31, 2019

topper-123 Dec 31, 2019 • edited Loading

Choose a reason for hiding this 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

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

pep8speaks commented Dec 31, 2019 • edited Loading

Comment last updated at 2020-01-05 12:20:24 UTC

topper-123 commented Dec 31, 2019 • edited Loading

simonjayhawkins commented Dec 31, 2019

topper-123 commented Dec 31, 2019 • edited Loading

simonjayhawkins commented Dec 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Dec 31, 2019 via email

topper-123 commented Dec 31, 2019

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

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

Choose a reason for hiding this comment

topper-123 Jan 1, 2020 • edited Loading

Choose a reason for hiding this comment

topper-123 Jan 1, 2020 • edited Loading

Choose a reason for hiding this comment

topper-123 commented Jan 1, 2020

jreback left a comment

Choose a reason for hiding this comment

WillAyd commented Jan 2, 2020

topper-123 commented Jan 2, 2020 • edited Loading

simonjayhawkins commented Jan 2, 2020

topper-123 commented Jan 2, 2020 • edited Loading

jreback commented Jan 3, 2020

topper-123 commented Jan 5, 2020

topper-123 commented Jan 5, 2020

jreback commented Jan 5, 2020

topper-123 Dec 31, 2019 •

edited

Loading

pep8speaks commented Dec 31, 2019 •

edited

Loading

topper-123 commented Dec 31, 2019 •

edited

Loading

topper-123 commented Dec 31, 2019 •

edited

Loading

topper-123 Jan 1, 2020 •

edited

Loading

topper-123 Jan 1, 2020 •

edited

Loading

topper-123 commented Jan 2, 2020 •

edited

Loading

topper-123 commented Jan 2, 2020 •

edited

Loading