ENH: Support EAs in Series.unstack
State: closed

Closes https://github.com/pandas-dev/pandas/issues/23077

This prevents ExtensionArray-backed series from being converted to object-dtype in unstack.

The strategy is to do a dummy unstack on an ndarray of integers, which provides the `indices` to `take` later on. We then concat together at the end. This provided decent performance, and seems pretty maintainable in the long run.

I'll post some benchmarks later.

Do we want to do DataFrame.stack() in the same PR?

Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

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

 - There are no PEP8 issues in the file [`pandas/core/reshape/reshape.py`](https://github.com/TomAugspurger/pandas/blob/942db1b918f818281c5dcfd10a951e519a04dc42/pandas/core/reshape/reshape.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/base/reshaping.py`](https://github.com/TomAugspurger/pandas/blob/942db1b918f818281c5dcfd10a951e519a04dc42/pandas/tests/extension/base/reshaping.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/decimal/array.py`](https://github.com/TomAugspurger/pandas/blob/942db1b918f818281c5dcfd10a951e519a04dc42/pandas/tests/extension/decimal/array.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/json/test_json.py`](https://github.com/TomAugspurger/pandas/blob/942db1b918f818281c5dcfd10a951e519a04dc42/pandas/tests/extension/json/test_json.py) !



##### Comment last updated on October 22, 2018 at 21:41 Hours UTC
Just fixed the decimal failures.

There will be a remaining test failure I haven't addressed yet. We had a test that did `Series[categorical].unstack(fill_value=value)` for a value that wasn't part of the original `categories` in the categorical. We aren't `take` correctly right now (https://github.com/pandas-dev/pandas/issues/23296), but once that's fixed there's still an API discussion: should we allow `take` to take "new" categories that weren't previously present.

```python
In [2]: cat = pd.Categorical(['a', 'a', 'b'])

In [3]: cat.take([0, -1, -1], fill_value='d', allow_fill=True)
```

Should that raise? Return a Categorical with categories `['a', 'b', 'd']`?

I'm having some deja vu right now; I think we've discussed this before.

I think if we were designing that today, we wouldn't have allowed that. 
> I think if we were designing that today, we wouldn't have allowed that.

I'm actually rethinking, this. Maybe we *would* want to allow this. It's a pretty clear statement of user intent, and I could easily imaging someone wanting to do something like "take, but fill missing values (-1) with "None" or "other".
Do you want to resolve the fill_value` question here, or leave for https://github.com/pandas-dev/pandas/issues/23296 ?

(as I mentioned there: I would preserve the dtype, which then means only allowing a fill_value that is NaN or within the categories)
We can ignore `fill_value` in this PR. Though of course we *should* discuss the API ramifications of `unstack` preserving the dtype.

If we agree that `Categorical.take` should not allow new categories for `fill_value`, I think we have two options

1. Not allow `Series.unstack(..., fill_value=fill_value)` to be a new category. Raise a TypeError instead.
2. Allow `Series.unstack`(..., fill_value=fill_value)` to be a new category by adding it to the CategoricalDtype before `take`ing.

I'm not sure which is preferred.

This is blocked by https://github.com/pandas-dev/pandas/issues/23296 for now.
Apparently, `fill_value` was broken in 0.23.4 for Categortical

```python
In [51]: index = pd.MultiIndex.from_tuples([('A', 0), ('A', 1), ('B', 1)])

In [52]: ser = pd.Series(pd.Categorical(['a', 'b', 'a']), index=index)

In [53]: ser.unstack(fill_value='c')
Out[53]:
     0  1
A    a  b
B  NaN  a

In [54]: ser.unstack(fill_value='a')
Out[54]:
   0  1
A  a  b
B  a  a
```

We just silently didn't fill the `'d'`. So I guess we just raise an error there instead?
> So I guess we just raise an error there instead?

+1
https://github.com/pandas-dev/pandas/pull/10246/files#diff-79e0785420ae1c686623848c4d561486R261 indicates that this was deliberate, but I didn't see any discussion / documentation around it, so I'm calling it a bug.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23284?src=pr&el=h1) Report
> Merging [#23284](https://codecov.io/gh/pandas-dev/pandas/pull/23284?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/6bf6cd2fe991f4f57e045e1f28a9dff860b72b37?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `97.29%`.

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

```diff
@@            Coverage Diff             @@
##           master   #23284      +/-   ##
==========================================
- Coverage   92.25%   92.23%   -0.02%     
==========================================
  Files         161      161              
  Lines       51186    51198      +12     
==========================================
+ Hits        47222    47224       +2     
- Misses       3964     3974      +10
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.62% <97.29%> (-0.02%)` | :arrow_down: |
| #single | `42.27% <27.02%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23284?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/internals/managers.py](https://codecov.io/gh/pandas-dev/pandas/pull/23284/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL21hbmFnZXJzLnB5) | `95.74% <100%> (ø)` | :arrow_up: |
| [pandas/core/reshape/reshape.py](https://codecov.io/gh/pandas-dev/pandas/pull/23284/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9yZXNoYXBlLnB5) | `99.54% <100%> (-0.01%)` | :arrow_down: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/23284/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `93.67% <95%> (-0.36%)` | :arrow_down: |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/23284/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `97.35% <0%> (-0.67%)` | :arrow_down: |
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/23284/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `95.83% <0%> (-0.27%)` | :arrow_down: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/23284/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.09% <0%> (-0.13%)` | :arrow_down: |
| [pandas/core/arrays/sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/23284/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3NwYXJzZS5weQ==) | `91.71% <0%> (-0.13%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23284?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/23284?src=pr&el=footer). Last update [6bf6cd2...56e5f2f](https://codecov.io/gh/pandas-dev/pandas/pull/23284?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

I also recall some issues that might be closed by this (haven't looked), but categorical reshaping might be a good place to start.
FYI, my comment about this changing behavior for Series / categorical wasn't correct. This will be backwards compat, we're just changing it so that sparse & categorical are no longer special-cased to work.

`DataFrame.unstack()` is proving to be a bit of work, but I should get that done today.
Things are a bit better in 5902b5b.
Failure in https://travis-ci.org/pandas-dev/pandas/jobs/445842263 is unrelated (fixed in https://github.com/pandas-dev/pandas/pull/23323)
Here are the results from that ASV

master + benchmark:

```
[  0.00%] ·· Benchmarking existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3
[ 25.00%] ··· Running (reshape.Unstack.time_full_product--)..
[ 75.00%] ··· reshape.Unstack.time_full_product                                                                                                           ok
[ 75.00%] ··· ========== ==========
                param1
              ---------- ----------
                 int      73.0±2ms
               category   193±1ms
              ========== ==========

[100.00%] ··· reshape.Unstack.time_without_last_row                                                                                                       ok
[100.00%] ··· ========== ===========
                param1
              ---------- -----------
                 int      189±0.7ms
               category    509±5ms
              ========== ===========
```

and this PR

```
[  0.00%] ·· Benchmarking existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3
[ 25.00%] ··· Running (reshape.Unstack.time_full_product--)..
[ 75.00%] ··· reshape.Unstack.time_full_product                                                                                                           ok
[ 75.00%] ··· ========== ============
                param1
              ---------- ------------
                 int      75.9±0.4ms
               category    192±4ms
              ========== ============

[100.00%] ··· reshape.Unstack.time_without_last_row                                                                                                       ok
[100.00%] ··· ========== =========
                param1
              ---------- ---------
                 int      190±2ms
               category   515±2ms
              ========== =========

```


The int case should be unchanged from before. The only thing that should differ there is an extra `if is_extension_dtype` check.

The category one should be taking the new code path, but those timings are suspiciously close together that I'm going to double check.
Ugh, yeah I messed up the ASV. Forgot to convert to categorical.

```
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3
[ 25.00%] ··· reshape.Unstack.time_full_product                                                                                       ok
[ 25.00%] ··· ========== =========
                param1
              ---------- ---------
                 int      137±0ms
               category   17.2±0s
              ========== =========

[ 50.00%] ··· reshape.Unstack.time_without_last_row                                                                                   ok
[ 50.00%] ··· ========== =========
                param1
              ---------- ---------
                 int      265±0ms
               category   17.5±0s
              ========== =========

```

Back to the drawing board...
Ah, I forgot to re-run the benchmark on master after converting to category. So we're actually faster!

Master:

```
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3
[ 25.00%] ··· reshape.Unstack.time_full_product                                                                                                           ok
[ 25.00%] ··· ========== =========
                param1
              ---------- ---------
                 int      136±0ms
               category   50.6±0s
              ========== =========

[ 50.00%] ··· reshape.Unstack.time_without_last_row                                                                                                       ok
[ 50.00%] ··· ========== =========
                param1
              ---------- ---------
                 int      263±0ms
               category   49.6±0s
              ========== =========
```

This branch

```
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3
[ 25.00%] ··· reshape.Unstack.time_full_product                                                                                                           ok
[ 25.00%] ··· ========== =========
                param1
              ---------- ---------
                 int      141±0ms
               category   18.0±0s
              ========== =========

[ 50.00%] ··· reshape.Unstack.time_without_last_row                                                                                                       ok
[ 50.00%] ··· ========== =========
                param1
              ---------- ---------
                 int      257±0ms
               category   18.0±0s
              ========== =========

```

I'm going to update the benchmark to not take 18s though.
This should be good to go, assuming CI passes.
K, I'll fix the `pd.isna(Decimal('NaN'))` and revert the other changes.
Hmm, fixing `isna` for decimal seems to be expensive. `np.isnan` raises on decimal input, so we would need to search any object array for decimal elements, and call our isna on it. I don't think that's a good idea.
> Hmm, fixing isna for decimal seems to be expensive. np.isnan raises on decimal input, so we would need to search any object array for decimal elements, and call our isna on it. I don't think that's a good idea.

this would only be for object input, and can you all ``math.isnan``? (maybe instead)?
Ahh I thought we would have to pay the extra cost on string dtypes too, but it seems like those are handled before we get to a generic object dtype. This should be doable.

Well, I'm going back to -1 on support decimal here, unless we can find a better way than a basic isnstance.

```diff
diff --git a/doc/source/whatsnew/v0.24.0.txt b/doc/source/whatsnew/v0.24.0.txt
index f449ca532..c8c5db611 100644
--- a/doc/source/whatsnew/v0.24.0.txt
+++ b/doc/source/whatsnew/v0.24.0.txt
@@ -1227,6 +1227,7 @@ Missing
 - Bug in :func:`Series.hasnans` that could be incorrectly cached and return incorrect answers if null elements are introduced after an initial call (:issue:`19700`)
 - :func:`Series.isin` now treats all NaN-floats as equal also for `np.object`-dtype. This behavior is consistent with the behavior for float64 (:issue:`22119`)
 - :func:`unique` no longer mangles NaN-floats and the ``NaT``-object for `np.object`-dtype, i.e. ``NaT`` is no longer coerced to a NaN-value and is treated as a different entity. (:issue:`22295`)
+- :meth:`isna` now considers ``decimal.Decimal('NaN')`` a missing value (:issue:`23284`).
 
 
 MultiIndex
diff --git a/pandas/_libs/missing.pyx b/pandas/_libs/missing.pyx
index b87913592..4fa96f652 100644
--- a/pandas/_libs/missing.pyx
+++ b/pandas/_libs/missing.pyx
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 
 import cython
+import decimal
 from cython import Py_ssize_t
 
 import numpy as np
@@ -33,6 +34,8 @@ cdef inline bint _check_all_nulls(object val):
         res = get_datetime64_value(val) == NPY_NAT
     elif util.is_timedelta64_object(val):
         res = get_timedelta64_value(val) == NPY_NAT
+    elif isinstance(val, decimal.Decimal):
+        return val.is_nan()
     else:
         res = 0
     return res
@@ -71,6 +74,8 @@ cpdef bint checknull(object val):
         return get_timedelta64_value(val) == NPY_NAT
     elif util.is_array(val):
         return False
+    elif isinstance(val, decimal.Decimal):
+        return val.is_nan()
     else:
         return val is None or util.is_nan(val)
 
```

some timings

kind          | master | PR     | ratio
------------- | ------ | ------ | -----
scalar        | 821 ns | 920 ns | 1.12 (different result)
object array  | 1.0 ms | 2.4 ms | 2.4
decimal array | 1.0 ms | 3.6 ms | 3.6 (different result)

the object array is an object-dtype series with 20,000 elements. The decimal array is an object-dtype series with 20,000 decimal elements. I don't really care about the last one being 3.6x slower, since we're getting the correct result. I'm more concerned about the others.
this moves calls to python land. Try

```
elif hasattr(val, 'is_nan'):
    return val.is_nan()
```
According to https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#built-in-functions both hasattr and isintsance are optimized (but require some interaction with python land).



kind          | master | PR     | ratio
------------- | ------ | ------ | -----
scalar        | 821 ns | 926 ns | 1.12 (different result)
object array  | 1.0 ms | 6.6 ms | 6.6
decimal array | 1.0 ms | 2.0 ms | 2.0  (different result)


so pretty similar. I don't really know why the object array would be 6x slower now though.
hmm i guess just the additional check is causing this.

but a more general questions. *should* we even be checking for this in an ndarray object array at all? e.g. we don't do this for a random foo object. It must be a decimal array (in which case you just ask it .isnan).?
Shall we leave possible changes/support for decimal in the internals for another issue or PR? 
Because in some sense it is exactly the idea of an ExtensionArray that it can override the behaviour of `pd.isna` to do this correctly no?
> Shall we leave possible changes/support for decimal in the internals for another issue or PR?
Because in some sense it is exactly the idea of an ExtensionArray that it can override the behaviour of pd.isna to do this correctly no?

totally fine. @TomAugspurger can you xfail these tests rather than change them though. and create an issue to update.
> can you xfail these tests rather than change them though. and create an issue to update.

Isn't the current code fine? It's contained in `test_decimal.py`, I don't think it is a problem that there is some decimal specific code in that file?
https://github.com/pandas-dev/pandas/issues/23530 for isna(decimal).

Fixed the merge conflict.
All green.