TYPING: Partial typing of Categorical #27318

topper-123 · 2019-07-10T06:29:45Z

Adds some typing to Categorical.

This PR does the simpler typing of Categorical. In addition there are some more thorny typing that I've stayed away from for now to see if others can implement the basic types (list-likes etc.).

topper-123 · 2019-07-10T06:30:50Z

pandas/_typing.py

@@ -26,8 +26,11 @@
 )
 ArrayLike = TypeVar("ArrayLike", ABCExtensionArray, np.ndarray)
 DatetimeLikeScalar = TypeVar("DatetimeLikeScalar", Period, Timestamp, Timedelta)
-Dtype = Union[str, np.dtype, ExtensionDtype]
+Dtype = Union[str, np.dtype, "dtypes.ExtensionDtype"]


If I don't do it like this, I get a circular import error.

Does this actually resolve back to "ExtensionDtype" or does it replace it with Any?

Yes, it works in PyCharm + I tried setting the string to a random string, which mypy won't accept.

Good to know. Wondering if we can extend that to solve #27146 (separate PR obviously)

Yeah, I think that should be possible, though it wold be even better if the ABCs could be used...

can't you use 'ExtendionDtype' here?

hmm I guess maybe revert to what you had before is more clear (e.g. do't use the string)

topper-123 · 2019-07-10T06:31:48Z

pandas/core/arrays/categorical.py

@@ -519,6 +521,8 @@ def astype(self, dtype, copy=True):

        """
        if is_categorical_dtype(dtype):
+            dtype = cast(Union[str, CategoricalDtype], dtype)


mypy is not able to infer the dtype type here, so I have to do a cast.

For a typing dummy, can you explain why this is needed?

I can add something to the contributing guide about this

WillAyd

Thanks for keeping these going

WillAyd · 2019-07-10T12:48:21Z

pandas/_typing.py

@@ -26,8 +26,11 @@
 )
 ArrayLike = TypeVar("ArrayLike", ABCExtensionArray, np.ndarray)
 DatetimeLikeScalar = TypeVar("DatetimeLikeScalar", Period, Timestamp, Timedelta)
-Dtype = Union[str, np.dtype, ExtensionDtype]
+Dtype = Union[str, np.dtype, "dtypes.ExtensionDtype"]


Does this actually resolve back to "ExtensionDtype" or does it replace it with Any?

WillAyd · 2019-07-10T12:50:29Z

pandas/_typing.py

 FilePathOrBuffer = Union[str, Path, IO[AnyStr]]

 FrameOrSeries = TypeVar("FrameOrSeries", ABCSeries, ABCDataFrame)
 Scalar = Union[str, int, float]
+
+# TODO(GH26403): Replace with Optional[bool] or bool
+OrderedType = Union[None, bool, object]


Always go for Optional instead of Union[None...].

Also object is very rarely what you want in Typing so would want to use Any instead, though the Union of Any and another object also doesn't make sense since Any encompasses everything...

Can't this just be Optional[bool]?

Also reading through the entire diff I see you just moved this from elsewhere so no worries but makes sense to correct here

Optional only works with a single argument, so we have to use Union here.

The object type will also be removed in 1.0, so this type can just be bool. I think @TomAugspurger will deal with that. If you look at the comment in core/dtypes/dtypes.py, line 21 to see the reasoning.

Gotcha but the sentinel never actually gets returned from def ordered below right? If that's correct then I think should be Ordered = Optional[bool] and in the init for CategoricalDtype should be Union[Ordered, object]

Unfortunately I can't get that to work. Categorical.ordered's type is same as CategoricalDtype._ordered which typewise may be a object (but actually can't be).

I don't have a good solution, except waiting for ordered_sentinel to be removed in 1.0?

Can you share the error it gives you? If the problem is that the ordered_sentinel is in the constructor of CategoricalDtype I think could just explicitly annotate that parameter to be Optional[bool] as well, which would be cleanest in the long run

pandas\core\arrays\categorical.py:475: error: Incompatible return value type (got "object", expected "Optional[bool]")

It all goes back to the ordered parameter being of type Union[None, bool, object], while the ordered attribute should be of type Optional[bool] (but isn't, as its type comes from the parameter's type).

Don't know if there's a pattern for having different parameter and attribute types...

Can you just add # type: ignore to that one line? I think that is the lesser of all evils and easiest to revert whenever that change gets made

Very nitpicky but can you also change the name from OrderedType? That would seem to imply that this is something like a TypeVar or NewType.

Either OrderedAlias or just plain Ordered would be a better fit

WillAyd · 2019-07-10T14:32:44Z

pandas/_typing.py

@@ -26,8 +26,11 @@
 )
 ArrayLike = TypeVar("ArrayLike", ABCExtensionArray, np.ndarray)
 DatetimeLikeScalar = TypeVar("DatetimeLikeScalar", Period, Timestamp, Timedelta)
-Dtype = Union[str, np.dtype, ExtensionDtype]
+Dtype = Union[str, np.dtype, "dtypes.ExtensionDtype"]


Good to know. Wondering if we can extend that to solve #27146 (separate PR obviously)

WillAyd · 2019-07-10T14:36:03Z

pandas/_typing.py

 FilePathOrBuffer = Union[str, Path, IO[AnyStr]]

 FrameOrSeries = TypeVar("FrameOrSeries", ABCSeries, ABCDataFrame)
 Scalar = Union[str, int, float]
+
+# TODO(GH26403): Replace with Optional[bool] or bool
+OrderedType = Union[None, bool, object]


Gotcha but the sentinel never actually gets returned from def ordered below right? If that's correct then I think should be Ordered = Optional[bool] and in the init for CategoricalDtype should be Union[Ordered, object]

WillAyd · 2019-07-10T20:27:50Z

pandas/_typing.py

 FilePathOrBuffer = Union[str, Path, IO[AnyStr]]

 FrameOrSeries = TypeVar("FrameOrSeries", ABCSeries, ABCDataFrame)
 Scalar = Union[str, int, float]
+
+# TODO(GH26403): Replace with Optional[bool] or bool
+OrderedType = Union[None, bool, object]


Can you just add # type: ignore to that one line? I think that is the lesser of all evils and easiest to revert whenever that change gets made

WillAyd · 2019-07-10T20:28:55Z

pandas/_typing.py

 FilePathOrBuffer = Union[str, Path, IO[AnyStr]]

 FrameOrSeries = TypeVar("FrameOrSeries", ABCSeries, ABCDataFrame)
 Scalar = Union[str, int, float]
+
+# TODO(GH26403): Replace with Optional[bool] or bool
+OrderedType = Union[None, bool, object]


Very nitpicky but can you also change the name from OrderedType? That would seem to imply that this is something like a TypeVar or NewType.

Either OrderedAlias or just plain Ordered would be a better fit

WillAyd · 2019-07-10T20:46:02Z

pandas/core/dtypes/dtypes.py

@@ -547,6 +548,8 @@ def update_dtype(self, dtype: "CategoricalDtype") -> "CategoricalDtype":
            ).format(dtype=dtype)
            raise ValueError(msg)

+        dtype = cast(CategoricalDtype, dtype)


Can you move this below subsequent comment? Seems to read better and more logically fit there

As an aside I was kind of curious why this couldn't be inferred more easily since it's really just a str and a CategoricalDtype that are allowed here. So ideally I thought isinstance(str) and isinstance(self) would be two possible branches.

I think the reason this wasn't laid out that way is to allow Categorical objects to be passed as well, but doing so actually causes an error:

t = pd.CategoricalDtype(categories=['b', 'a'], ordered=True) c = pd.Categorical(['a','b','c','a','b','c'], ordered=True, categories=['c', 'b', 'a']) >>> t.update_dtype(c) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/williamayd/clones/pandas/pandas/core/dtypes/dtypes.py", line 561, in update_dtype new_ordered = dtype._ordered AttributeError: 'Categorical' object has no attribute '_ordered'

cc @TomAugspurger for intent. If a bug maybe we drop this annotation for now and clean up with bug resolution

That's a bug in update_dtype due to the changes we recently made with ordered cc @jschendel

The code in there is assuming we have a CategoricalDtype (which has a _ordered now), but it can apparently also be a Categorical (or should that raise?), because is_dtype allows that.

Cool thanks - for now I think OK to remove annotation from this PR then and can be addressed with whatever bug fix is there

Opened #27338 to address this

jreback · 2019-07-12T14:45:46Z

looks good, small comment, merge master and ping on green.

WillAyd · 2019-07-12T14:51:54Z

Can we address my comment on OrderedType before merging? I don't think the definition in pandas._typing of that should add object because one method in the code base uses that as a temporary workaround

jreback · 2019-07-23T22:27:44Z

can you rebase

topper-123 · 2019-07-25T00:28:25Z

Rebased.

jreback · 2019-07-25T17:21:20Z

lgtm. over to you @WillAyd

WillAyd · 2019-07-25T17:34:00Z

Great thanks a lot @topper-123

topper-123 commented Jul 10, 2019

View reviewed changes

topper-123 added Typing type annotations, mypy/pyright type checking Categorical Categorical Data Type labels Jul 10, 2019

topper-123 added this to the 0.25.0 milestone Jul 10, 2019

WillAyd requested changes Jul 10, 2019

View reviewed changes

WillAyd mentioned this pull request Jul 11, 2019

CategoricalDtype.update_dtype fails with Categorical #27338

Closed

jreback removed this from the 0.25.0 milestone Jul 17, 2019

TYPIMG: Partial typing of Categorical

0a3b2f7

topper-123 force-pushed the Categorical_typing branch from c3919fe to 5a6de11 Compare July 24, 2019 22:58

Changes

b76f27e

topper-123 force-pushed the Categorical_typing branch from 5a6de11 to b76f27e Compare July 24, 2019 23:41

jreback added this to the 1.0 milestone Jul 25, 2019

WillAyd approved these changes Jul 25, 2019

View reviewed changes

WillAyd merged commit 0a712fa into pandas-dev:master Jul 25, 2019

quintusdias pushed a commit to quintusdias/pandas_dev that referenced this pull request Aug 16, 2019

TYPING: Partial typing of Categorical (pandas-dev#27318)

8979c5b

TYPING: Partial typing of Categorical #27318

TYPING: Partial typing of Categorical #27318

Conversation

topper-123 commented Jul 10, 2019 • 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

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

WillAyd 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

topper-123 Jul 10, 2019 • 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

topper-123 Jul 10, 2019 • 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

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 commented Jul 12, 2019

WillAyd commented Jul 12, 2019

jreback commented Jul 23, 2019

topper-123 commented Jul 25, 2019

jreback commented Jul 25, 2019

WillAyd commented Jul 25, 2019

topper-123 commented Jul 10, 2019 •

edited

Loading

topper-123 Jul 10, 2019 •

edited

Loading

topper-123 Jul 10, 2019 •

edited

Loading