Add type hints to dtypes/dtypes.py (CategoricalDtype) #26327

topper-123 · 2019-05-09T16:36:31Z

This PR adds type hints to pandas/core/dtypes/dtypes.py down to and including the CategoricalDtype part of the module. I'm more familiar with Categoricals than DateTimeArrays etc. and because I think there are some difficulties in implementing type hints, I think it would make sense to add a general ruleset for adding type hints to pandas, before too much work is put into this.

I'm putting some thoughts up for discussion in #25601 about how we add type hints to pandas, that probably should be discussed before this goes further.

codecov · 2019-05-09T17:14:39Z

Codecov Report

Merging #26327 into master will decrease coverage by 51.29%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26327      +/-   ##
==========================================
- Coverage   92.04%   40.74%   -51.3%     
==========================================
  Files         175      175              
  Lines       52297    52304       +7     
==========================================
- Hits        48138    21313   -26825     
- Misses       4159    30991   +26832

Flag	Coverage Δ
#multiple	`?`
#single	`40.74% <100%> (-0.14%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/categorical.py	`41.36% <100%> (-54.6%)`	⬇️
pandas/core/dtypes/dtypes.py	`73.7% <100%> (-22.72%)`	⬇️
pandas/io/formats/latex.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas_constants.py	`0% <0%> (-100%)`	⬇️
pandas/core/groupby/categorical.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/plotting.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/converter.py	`0% <0%> (-100%)`	⬇️
pandas/io/formats/html.py	`0% <0%> (-99.37%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-91.16%)`	⬇️
pandas/io/sas/sas_xport.py	`0% <0%> (-90.1%)`	⬇️
... and 132 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17247ed...69b4bb1. Read the comment docs.

codecov · 2019-05-09T17:14:55Z

Codecov Report

❗ No coverage uploaded for pull request base (master@de0867f). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #26327   +/-   ##
=========================================
  Coverage          ?   92.04%           
=========================================
  Files             ?      175           
  Lines             ?    52304           
  Branches          ?        0           
=========================================
  Hits              ?    48142           
  Misses            ?     4162           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.59% <100%> (?)`
#single	`40.75% <100%> (?)`

Impacted Files	Coverage Δ
pandas/core/arrays/categorical.py	`95.96% <100%> (ø)`
pandas/core/dtypes/dtypes.py	`96.71% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de0867f...8f6c2e0. Read the comment docs.

topper-123 · 2019-05-09T17:23:35Z

pandas/core/dtypes/dtypes.py

        """
        Parameters
        ----------
        dtype : ExtensionDtype
        """
-        if not issubclass(dtype, (PandasExtensionDtype, ExtensionDtype)):
+        if not issubclass(dtype, ExtensionDtype):


All sublcasses of PandasExtensionDtype are now also ExtensionDtype, so I think this check is not needed anymore.

@TomAugspurger, do you agree?

Sounds correct.

WillAyd

Thanks for this! FYI this is the first PR to add annotations without fixing an existing type failure, so might take a few iterations to work out both the contribution and review side of things

pandas/core/arrays/categorical.py

WillAyd · 2019-05-09T21:19:56Z

pandas/core/dtypes/dtypes.py

@@ -1,13 +1,15 @@
 """ define extension dtypes """
 import re
-from typing import Any, Dict, Optional, Tuple, Type
+import typing


The preferred idiom is to just use from typing import ... so can add cast to the below line (if needed)

Ok. Not even sure using cast is correct, it feels ugly.

remove the typing, just import as indicated

WillAyd · 2019-05-09T21:20:55Z

pandas/core/dtypes/dtypes.py

@@ -18,7 +20,8 @@
 str_type = str


-def register_extension_dtype(cls):
+def register_extension_dtype(cls: Type['ExtensionDtype'],


Same comment on referencing object instead of using a reference

agreed, pls change this

pandas/core/dtypes/dtypes.py

WillAyd · 2019-05-09T21:23:21Z

pandas/core/dtypes/dtypes.py

        return self.name

-    def __str__(self):
+    @property


What error was this causing?

It was an AttributeError, caused by the call to .name in def __unicode__

Do subclasses assign name as a property or a class variable? If not the former then doing it this way will cause typing issue in the subclasses

WillAyd · 2019-05-09T21:25:06Z

pandas/core/dtypes/dtypes.py

        raise NotImplementedError("sub-classes should implement an __hash__ "
                                  "method")

-    def __getstate__(self):
+    def __getstate__(self) -> Dict[str_type, Any]:


Would like to minimize the use of Any as much as possible. Can't the value here not be str as well? May need to add type to self._metadata to really make use of this

I think Dict[str, Union[Optional[Index], bool]] will do it.

__getstate__ is also used by subclasses, so the strings in _metadata that I find are needed are (with associated guess for types for attributes):

"categories": Optional[Index]

"ordered": Optional[bool]

"unit": str (+other?)

"tz": str, I think (+ others?)

"freq": Not sure ATM

"subtype": Not sure ATM (_typing.Dtype?)

If the type hints are important to be specific to the ExtensionsDtype, maybe subclass __getstate__ in order to set the correct type hints? (in different PR IMO)?

WillAyd · 2019-05-09T21:25:35Z

pandas/core/dtypes/dtypes.py

        self._finalize(categories, ordered, fastpath=False)

    @classmethod
-    def _from_fastpath(cls, categories=None, ordered=None):
+    def _from_fastpath(cls,
+                       categories=None,


Can you annotate categories?

Only imprecisely. The most similar would be a plain Iterable, but that would accept e.g. "categories='abc'", which would ideally flag as a wrong type in mypy, but that's not possible ATM.

I'm a bit apprehensive about accepting wrong types, but maybe that would be the correct compromise.

categories should be ListLike which is ArrayLike + list + tuple + Iterable (define it in _typing)

WillAyd · 2019-05-09T21:25:50Z

pandas/core/dtypes/dtypes.py

-    def _from_values_or_dtype(cls, values=None, categories=None, ordered=None,
-                              dtype=None):
+    def _from_values_or_dtype(cls,
+                              values=None,


Can you annotate values / categories?

See comment above. The most precise here would again be Iterable, but same problems as above.

TomAugspurger · 2019-05-10T03:12:51Z

pandas/core/dtypes/dtypes.py


    @property
-    def ordered(self):
+    def ordered(self) -> Optional[bool]:


Is this optional? I thought ordered was always sure or values (categories are optional).

Yes:

In [1]: pd.CategoricalDtype() Out[1]: CategoricalDtype(categories=None, ordered=None)

I think this has somwthing to to with updating dtypes (don't update None, but do update False).

grr, this isn't needed, and isn't how we document things. #26336

pandas/core/dtypes/dtypes.py

pandas/core/arrays/categorical.py

jreback · 2019-05-11T19:10:17Z

pandas/core/dtypes/dtypes.py

@@ -1,13 +1,15 @@
 """ define extension dtypes """
 import re
-from typing import Any, Dict, Optional, Tuple, Type
+import typing


remove the typing, just import as indicated

jreback · 2019-05-11T19:10:29Z

pandas/core/dtypes/dtypes.py

@@ -18,7 +20,8 @@
 str_type = str


-def register_extension_dtype(cls):
+def register_extension_dtype(cls: Type['ExtensionDtype'],


agreed, pls change this

jreback · 2019-05-11T19:11:22Z

pandas/core/dtypes/dtypes.py

@@ -126,16 +131,20 @@ class PandasExtensionDtype(_DtypeOpsMixin):
    isnative = 0
    _cache = {}  # type: Dict[str_type, 'PandasExtensionDtype']

-    def __unicode__(self):
+    def __unicode__(self) -> str_type:


not sure what this is, str should work here

jreback · 2019-05-11T19:12:06Z

pandas/core/dtypes/dtypes.py

-    def __str__(self):
+    @property
+    def name(self) -> str_type:
+        raise AbstractMethodError(self)


why are you using str_type? just str

I think str_type came about in a previous PR because the class has a variable also called str which was conflicting with the builtin.

@gwrome

that is not a good idea at all. pls revert that

So the other option that was proposed was to namespace it as builtins.str within the scope of the class - do you prefer that?

The discussions about str getting shadowed were in #26029, for reference. I still prefer the explicit builtins.str approach.

jreback · 2019-05-11T19:14:02Z

pandas/core/dtypes/dtypes.py

        self._finalize(categories, ordered, fastpath=False)

    @classmethod
-    def _from_fastpath(cls, categories=None, ordered=None):
+    def _from_fastpath(cls,
+                       categories=None,


categories should be ListLike which is ArrayLike + list + tuple + Iterable (define it in _typing)

jreback · 2019-06-08T20:31:31Z

@topper-123 can you merge master

jreback · 2019-06-28T14:41:34Z

pandas/core/dtypes/dtypes.py

@@ -126,28 +130,28 @@ class PandasExtensionDtype(ExtensionDtype):
    isnative = 0
    _cache = {}  # type: Dict[str_type, 'PandasExtensionDtype']

-    def __str__(self):
+    def __str__(self) -> str_type:


@WillAyd on your how-to-type-annotate can you add when / why to use str_type

jreback · 2019-06-28T14:42:43Z

@WillAyd ok with merging this, though maybe should fix the import itself, you commented?) anything else?

WillAyd · 2019-06-28T15:08:58Z

taking a look

WillAyd · 2019-06-28T15:22:27Z

Simplified a few things and removed the casts. OK to merge if green

jreback · 2019-06-28T16:25:39Z

thanks @topper-123

I think we can deal with the .ordered annotation later, cc @jschendel

topper-123 force-pushed the typing_dtypes.py branch from 2c32937 to 01b3ab8 Compare May 9, 2019 16:43

topper-123 mentioned this pull request May 9, 2019

Typing Master Tracker #25601

Closed

topper-123 force-pushed the typing_dtypes.py branch from 01b3ab8 to 69b4bb1 Compare May 9, 2019 17:14

topper-123 commented May 9, 2019

View reviewed changes

topper-123 force-pushed the typing_dtypes.py branch 2 times, most recently from 3580266 to ea08c99 Compare May 9, 2019 20:37

Add type hints to dtypes/dtypes.py (CategoricalDtype)

8be18a0

topper-123 force-pushed the typing_dtypes.py branch from ea08c99 to 8be18a0 Compare May 9, 2019 21:11

WillAyd added the Typing type annotations, mypy/pyright type checking label May 9, 2019

WillAyd requested changes May 9, 2019

View reviewed changes

TomAugspurger reviewed May 10, 2019

View reviewed changes

TomAugspurger mentioned this pull request May 10, 2019

Deprecate ordered=None for CategoricalDtype #26336

Closed

jreback requested changes May 11, 2019

View reviewed changes

jreback added 3 commits June 27, 2019 17:17

Merge branch 'master' into PR_TOOL_MERGE_PR_26327

8f6c2e0

Merge branch 'master' into PR_TOOL_MERGE_PR_26327

0fe837a

lint error

d0f150d

jreback reviewed Jun 28, 2019

View reviewed changes

jreback approved these changes Jun 28, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Jun 28, 2019

Cleaned up and removed casts

86666f0

jreback merged commit be4b48e into pandas-dev:master Jun 28, 2019

Add type hints to dtypes/dtypes.py (CategoricalDtype) #26327

Add type hints to dtypes/dtypes.py (CategoricalDtype) #26327

Conversation

topper-123 commented May 9, 2019 • edited Loading

codecov bot commented May 9, 2019

Codecov Report

codecov bot commented May 9, 2019 • edited Loading

Codecov Report

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

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

TomAugspurger May 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

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 Jun 8, 2019

Choose a reason for hiding this comment

jreback commented Jun 28, 2019

WillAyd commented Jun 28, 2019

WillAyd commented Jun 28, 2019

jreback commented Jun 28, 2019

topper-123 commented May 9, 2019 •

edited

Loading

codecov bot commented May 9, 2019 •

edited

Loading

TomAugspurger May 10, 2019 •

edited

Loading