BUG: Empty CategoricalIndex fails with boolean categories

[pganssle]

Fixes #22702.

This bug was introduced in 7818486859d1aba53; per my comment, the problem is here:

    if not is_dtype_equal(values.dtype, categories.dtype):
        values = ensure_object(values)
        categories = ensure_object(categories)

    (hash_klass, vec_klass), vals = _get_data_algo(values, _hashtables)
    (_, _), cats = _get_data_algo(categories, _hashtables)

When categories is Index([True], dtype='object') and values is array([], dtype='object'), the ensure_object call is bypassed, but in _get_data_algo, an Index consisting entirely of boolean objects will be coerced to uint64, which violates the assumption that values and categories have the same dtype.

I felt that retrieving the underlying numpy arrays (if any exist) is the safest way to handle this without having too many wide-reaching effects across the rest of the codebase, but there might be a better way to enforce that these are not coerced into different data types.

• closes #xxxx
• tests added
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[pep8speaks]

Hello @pganssle! Thanks for submitting the PR.

• There are no PEP8 issues in the file pandas/core/arrays/categorical.py !

• There are no PEP8 issues in the file pandas/tests/arrays/categorical/test_constructors.py !

[gfyoung]

Nice!

cc @TomAugspurger @jreback

[gfyoung]

Because this wasn't introduced in 0.23.x (but rather in 0.21.x), let's move this to 0.24.0.

[pganssle]

OK, moved.

[jreback]

I think you can just use np.asarray here as we have to re-infer anyhow. But pls check perf of this. The comment can be a bit simpler, and reference the gh issue number

[pganssle]

In [4]: categories = pd.Index([True, False], dtype='object')

In [5]: values = np.array([], dtype='object')

In [6]: %timeit np.asarray(values)
260 ns ± 1.41 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [7]: %timeit np.asarray(categories)
2.2 µs ± 21.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [8]: %timeit getattr(values, 'values', values)
312 ns ± 1.26 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [9]: %timeit getattr(categories, 'values', categories)
410 ns ± 4.23 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [10]: categories = pd.Index(list('abcdefghijklmnopqrstuvwxyz'), dtype='object')

In [11]: categories
Out[11]: 
Index(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n',
       'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'],
      dtype='object')

In [12]: %timeit np.asarray(categories)
2.09 µs ± 9.76 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [13]: %timeit getattr(categories, 'values', categories)
409 ns ± 6.44 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each

Looks like the way I've done it is faster, but np.asarray doesn't seem to be scaling with the number of categories, so I don't think it's a huge deal to use np.asarray. That said, I don't love the idea of using np.asarray in this situation, because although I can't think of a time when it would present a problem, np.asarray is allowed to infer dtype. The way I've done it you're guaranteed to either get the original thing back or the underlying numpy array, which is pretty much as light a touch as I can imagine.

[pganssle]

I've pushed new changes that address all comments (though I did not switch over to asarray).

[jreback]

tiny code change, otherwise lgtm. ping on green.

[jreback]

can you flip this block around, e.g.

if is_dtype_equal(...):
    # your added code
else:
    # ensure object

[pganssle]

I can do that if you want. One thing to note is that I believe if not is_dtype_equal case is probably the "common case", since it doesn't occur with many dtypes like integers or strings.

I don't think there's any speed advantage to being in the if branch vs. the else branch, but depending on convention you may prefer the common case to be the first block and the rare case to be the else block. Up to you.

[jreback]

its not about speed or anything else, its about consistency in our code of how we do these kinds of tests.

[codecov]

Codecov Report

Merging #22710 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22710      +/-   ##
==========================================
+ Coverage   92.17%   92.17%   +<.01%     
==========================================
  Files         169      169              
  Lines       50769    50771       +2     
==========================================
+ Hits        46798    46800       +2     
  Misses       3971     3971

Flag	Coverage Δ
#multiple	90.59% <100%> (ø)	⬆️
#single	42.32% <100%> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/categorical.py	95.75% <100%> (+0.01%)	⬆️

---

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 40dfadd...58c9d54. Read the comment docs.

[pganssle]

All issues should be addressed now. I'm guessing the azure pipeline failure is unrelated. I can rebase against master if it's fixed in master.

[TomAugspurger]

Currently working on the azure stuff, you can safely ignore it.

…

On Wed, Sep 19, 2018 at 9:04 AM Paul Ganssle ***@***.***> wrote: All issues should be addressed now. I'm guessing the azure pipeline failure is unrelated. I can rebase against master if it's fixed in master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22710 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIohEDLZwgt4Tp5nbgJeV3EhVf8sHks5uck7ogaJpZM4WpVL2> .

[jreback]

lgtm. merging after azure updates by @TomAugspurger

[TomAugspurger]

Sorry about all the noise, your PR is my guinea pig :)

Things should be OK now. Ping on green.

[pganssle]

@TomAugspurger Green now.

[TomAugspurger]

Thanks!