BUG: Fix potential segfault after pd.Categorical(pd.Series(...), categories=...)
State: closed

Closes #25318


Labels:
name: Regression
description: Functionality that used to work in a prior pandas version
name: Categorical
description: Categorical Data Type

Comments:
Hello @batterseapower! Thanks for updating the PR.





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on February 21, 2019 at 18:51 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=h1) Report
> Merging [#25368](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f74aba614554d1de4d7986fef6296a4eda951ac9?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25368/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #25368      +/-   ##
==========================================
- Coverage   91.73%   91.73%   -0.01%     
==========================================
  Files         173      173              
  Lines       52839    52837       -2     
==========================================
- Hits        48472    48470       -2     
  Misses       4367     4367
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.29% <100%> (-0.01%)` | :arrow_down: |
| #single | `41.72% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25368/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.96% <100%> (-0.02%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=footer). Last update [f74aba6...b28f798](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=h1) Report
> Merging [#25368](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/9c0f6a8d703b6bee48918f2c5d16418a7ff736e3?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25368/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #25368      +/-   ##
==========================================
+ Coverage   91.73%   91.73%   +<.01%     
==========================================
  Files         173      173              
  Lines       52845    52843       -2     
==========================================
- Hits        48479    48478       -1     
+ Misses       4366     4365       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.3% <100%> (-0.01%)` | :arrow_down: |
| #single | `41.72% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25368/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.96% <100%> (-0.02%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25368/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `87.66% <0%> (+0.09%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=footer). Last update [9c0f6a8...d74043b](https://codecov.io/gh/pandas-dev/pandas/pull/25368?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> pls show benchmarks

Replicating the benchmarks from #23814 I see very similar results.

HEAD:

```python
In [1]: import pandas as pd
In [2]: x = pd.Series(list('abcd') * 1000000).astype('category')
In [3]: %timeit x == 'a'
757 µs ± 9.88 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
````

After my change:
```python
1.21 ms ± 28.4 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
```

After my change but **without** the equal-categories fastpath:

```python
18.5 ms ± 193 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```

@batterseapower Just to clarify, what is the reason you timed `%timeit x == 'a'`. I would assume we should time `pd.Categorical(x)` ?
@jorisvandenbossche simply because that's the benchmark that motivated the original (broken) change that introduced the regression.
Ah, I see the original issue. But the PR that addressed it added a benchmark for `pd.Categorical(series)` and `pd.Categorical(categorical)`. So can you show those as well?
These are the `asv` changes for `categoricals.Constructor`

```
       before           after         ratio
     [54492791]       [a1b836a9]
     <master~1>       <master>  
+        48.3±1μs         268±10μs     5.55  categoricals.Constructor.time_existing_series
-        889±40μs         48.0±1μs     0.05  categoricals.Constructor.time_existing_categorical
```
Sorry, it is not fully clear to me what is master and what is this PR (master~1 is the PR?)

But in any case, one of the two situation (series or categorical) got slower with this PR, so we should look into how we can prevent that.
Yes, the `Series` case got slower, but I don't really see why this should be a problem? It's only fast in HEAD because we're doing something wildly unsafe! Surely the priority here should be fixing this regression rather than trying the preserve the illusory performance improvement from #23814
If I revert #23814 entirely i.e. don't add the extra check to `_recode_for_categories` then we get:

```
       before           after         ratio
     [54492791]       [4ebf932c]
     <master~2>       <master>  
+      39.5±0.8μs      1.29±0.07ms    32.70  categoricals.Constructor.time_existing_series
```

i.e. after this PR the `Series` case will still be 5x faster than it was before #23814, *and* the `Categorical` case will be 20x faster than `HEAD`. It might be tricky to match the 25x speedup for the `Series` case that #23814 was able to achieve by eliding all safety checks.
With this additional change:

```python
-            old_codes = (values.cat.codes if isinstance(values, ABCSeries)
+            old_codes = (values.values.codes if isinstance(values, ABCSeries)
```

We're fast enough that asv stops whining about the `time_existing_series` case:

```
       before           after         ratio
     [54492791]       [25aa03b2]
     <master~2>       <master>  
-        732±50μs       40.3±0.6μs     0.06  categoricals.Constructor.time_existing_categorical
```
lgtm.