BUG: Fix is_categorical_dtype for Sparse[category]

[dsaxton]

• closes BUG: is_categorical_dtype returns False for Sparse[category, nan] #35793
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[jreback]

pls perf test - this is a hot spot

[dsaxton]

Ran the categoricals.py asvs and somehow performance improved (?):

       before           after         ratio
     [97ec0627]       [b52a7eab]
     <master>         <sparse-categorical-dtype>
-      10.0±0.3ms       9.05±0.2ms     0.90  categoricals.Isin.time_isin_categorical('int64')
-      13.5±0.2ms         11.9±1ms     0.88  categoricals.Isin.time_isin_categorical('object')
-        5.75±2μs       4.00±0.1μs     0.69  categoricals.Indexing.time_get_loc

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

[dsaxton]

mypy error seems wrong, dtype.subtype comes after a hasattr(dtype, "subtype") check:

pandas/core/dtypes/base.py:292: error: "object" has no attribute "subtype"  [attr-defined]

Any ideas on how best to fix? Could explicitly ignore or maybe use something like isinstance(getattr(dtype, "subtype", None), cls)

[jorisvandenbossche]

Is this change actually something we necessarily want?

For example one use case where you might check that something is a categorical, is to know that you can then use certain APIs specific to categoricals. But if a Sparse type also gives true, that won't longer be true.

[dsaxton]

Is this change actually something we necessarily want?

For example one use case where you might check that something is a categorical, is to know that you can then use certain APIs specific to categoricals. But if a Sparse type also gives true, that won't longer be true.

Hmm, interesting point. I think the argument from the original issue seems valid though, that if Sparse[int] is an int, then Sparse[categorical] should be a categorical (making something sparse shouldn't change that). I suppose adopting that view would mean that there's really less of an API specific to categoricals, so some special casing might have to happen as a result.

I can see the point though, that this could just make things more confusing in other ways.

[jorisvandenbossche]

Yes, but as I noted on the issue: if there is an inconsistency, there are always two ways this can be solved. Meaning: we can also change Sparse[int] to no longer be int, or at least check what the use cases are for it being considered int (there quite possibly are, but I think we should try to be concrete first before deciding how to solve the inconsistency)

[dsaxton]

Meaning: we can also change Sparse[int] to no longer be int, or at least check what the use cases are for it being considered int (there quite possibly are, but I think we should try to be concrete first before deciding how to solve the inconsistency)

The more I think about it the more this seems like the better option over changing this for categorical; is_categorical_dtype loses most of it's usefulness if it doesn't guarantee access to the API specific to categoricals.

So then to be consistent sparse dtypes should really only register as sparse (not int, float, whatever)? I would be curious if that's actually easier internally, or what that might break if changed (or if you even bother changing it at all).

[simonjayhawkins]

mypy error seems wrong, dtype.subtype comes after a hasattr(dtype, "subtype") check:

pandas/core/dtypes/base.py:292: error: "object" has no attribute "subtype"  [attr-defined]

Any ideas on how best to fix? Could explicitly ignore or maybe use something like isinstance(getattr(dtype, "subtype", None), cls)

see python/mypy#1424

use #type: ignore and add a comment referencing the mypy issue

[dsaxton]

@jorisvandenbossche One possible counterpoint is that is_categorical_dtype already doesn't guarantee access to the categorical API:

[ins] In [1]: import pandas as pd

[ins] In [2]: from pandas.core.dtypes.common import is_categorical_dtype

[ins] In [3]: s = pd.Series(pd.Categorical(["a", "b", "c"]))

[ins] In [4]: is_categorical_dtype(s)
Out[4]: True

[ins] In [5]: s.codes
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-5-c9d8be9c51ce> in <module>
----> 1 s.codes

~/opt/miniconda3/envs/pandas-1.1.2/lib/python3.8/site-packages/pandas/core/generic.py in __getattr__(self, name)
   5134             if self._info_axis._can_hold_identifiers_and_holds_name(name):
   5135                 return self[name]
-> 5136             return object.__getattribute__(self, name)
   5137
   5138     def __setattr__(self, name: str, value) -> None:

AttributeError: 'Series' object has no attribute 'codes'

That assumption caused a bug here: #36383

[pep8speaks]

Hello @dsaxton! 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-09-19 13:01:13 UTC

[jreback]

do we really need to add this, this is a perf sensitive piece.

[dsaxton]

It's needed for test_is_categorical_sparse_categorical to pass; I'm not sure if there's another way to make this work. I ran the dtype asvs and didn't see a degradation.

[jreback]

code change looks ok. but I think @jorisvandenbossche is right here; if is_categorical_dtype is true then this should act like a categorical, which we currently don't do.

[jorisvandenbossche]

One possible counterpoint is that is_categorical_dtype already doesn't guarantee access to the categorical API:

But it does guarantee that on a Series .cat.codes is available (it's true you still need to know if it's a Series[Categorical] or Categorical).

Also, do we actually support Sparse[category] properly?
Eg I just took the example from the test you added, and the repr fails:

In [1]: s = pd.Series(["a", "b", "c"], dtype=pd.SparseDtype(pd.CategoricalDtype(["a", "b", "c"])))                                                                                                                 

In [2]: s                                                                                                                                                                                                          
...
ValueError: object __array__ method not producing an array

Some time ago I opened a general issue about the question to support Sparse backed by other ExtensionArrays or not: #26407

[dsaxton]

Closing since it seems there isn't a desire to change this