pivot_table very slow on Categorical data; how about an observed keyword argument? #24923
- [x] closes #24923
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24953?src=pr&el=h1) Report
> Merging [#24953](https://codecov.io/gh/pandas-dev/pandas/pull/24953?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/95f8dca47723192ddeed5a2f198d0521c687c9aa?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

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

```diff
@@           Coverage Diff           @@
##           master   #24953   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         166      166           
  Lines       52398    52398           
=======================================
  Hits        48406    48406           
  Misses       3992     3992
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.8% <100%> (ø)` | :arrow_up: |
| #single | `42.89% <0%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24953?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24953/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.92% <ø> (ø)` | :arrow_up: |
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/24953/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `96.55% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24953?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/24953?src=pr&el=footer). Last update [95f8dca...0662fa3](https://codecov.io/gh/pandas-dev/pandas/pull/24953?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/24953?src=pr&el=h1) Report
> Merging [#24953](https://codecov.io/gh/pandas-dev/pandas/pull/24953?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/d74901b1a114d70244fb919e8de630fb53481b00?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/24953/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24953?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24953      +/-   ##
==========================================
- Coverage   91.98%   91.98%   -0.01%     
==========================================
  Files         175      175              
  Lines       52372    52372              
==========================================
- Hits        48176    48173       -3     
- Misses       4196     4199       +3
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.53% <100%> (ø)` | :arrow_up: |
| #single | `40.7% <0%> (-0.16%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24953?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24953/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.9% <ø> (-0.12%)` | :arrow_down: |
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/24953/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `96.54% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/24953/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24953/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `90.71% <0%> (+0.1%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24953?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/24953?src=pr&el=footer). Last update [d74901b...3c1720c](https://codecov.io/gh/pandas-dev/pandas/pull/24953?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
 > you will be to use the observed fixture in some of the tests to validate the new behavior

I've added the observed fixture to the existing pivot_table test - let me know if this is better placed elsewhere. How far are we from a dict-encoded extension array? That would remove the need for an observed keyword.


________________________________
From: Benjamin Rowell <notifications@github.com>
Sent: Saturday, January 26, 2019 4:45 PM
To: pandas-dev/pandas
Cc: Subscribed
Subject: Re: [pandas-dev/pandas] pivot_table very slow on Categorical data; how about an observed keyword argument? #24923 (#24953)


you will be to use the observed fixture in some of the tests to validate the new behavior

I've added the observed fixture to the existing pivot_table test - let me know if this is better placed elsewhere.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/24953#issuecomment-457872386>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIuCe2421uAcbU02yvlSgP5hd3d6Iks5vHNqmgaJpZM4aUSqx>.
 > How far are we from a dict-encoded extension array? That would remove the need for an observed keyword.

looks easy, but I suspect a) this will be optional to start, and b) need quite a bit of testing. > looks easy, but I suspect a) this will be optional to start, and b) need quite a bit of testing.

What do you mean by optional?

I'd just prefer to not add additional keywords that'll be made irrelevant soon (almost surely within a year I would think). > I'd just prefer to not add additional keywords that'll be made irrelevant soon (almost surely within a year I would think).

we already have this for groupby and this totally makes sense. Even if you could guarantee it within a year I would *still* add this. i don't think we have an asv for pivoting with categorical data, can you add one representative of this example. Hello @benjaminr! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:









There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-04-23 15:21:45 UTC can you merge master. ping on green. can you merge master again - odd its still failing @jreback yeah I'm not sure what's causing this either. Seems to have happened since I added the asv. Is it the repr_html one that’s failing? I fixed that in another PR (on my phone so can’t link)

> On Feb 8, 2019, at 15:37, Benjamin Rowell <notifications@github.com> wrote:
> 
> @jreback yeah I'm not sure what's causing this either. Seems to have happened since I added the asv.
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.
 the Travis failure will be fixed if you merge master again! xref #25225. i've no idea what is causing the benchmark failure though. Was getting the ASV failure on another PR of mine. Opened #25235 for that specifically, which I think is separate from this change i merged master; ping no green. Looks like some more asv failures but this time a result of #25233 can you merge master and ping on green @jreback Seems like all is green apart from the Windows py27_np121 build which now appears to break on something Miniconda related. @benjaminr can you merge master again and fix conflict? @benjaminr we just dropped Py27 CI jobs so if that was only failure before try merging master to see if we are green @WillAyd seems to all pass apart from linting on some unrelated typing imports that are unused. Hmm that’s strange that that’s failing - can you try merging master again?

> On Mar 20, 2019, at 12:36 AM, Benjamin Rowell <notifications@github.com> wrote:
> 
> @WillAyd <https://github.com/WillAyd> seems to all pass apart from linting on some unrelated typing imports that are unused.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/pandas-dev/pandas/pull/24953#issuecomment-474719418>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOUcPAac-Eg72dODW3j_XOpuS6PSyhks5vYeUVgaJpZM4aUSqx>.
> 

William Ayd
william.ayd@icloud.com



 @WillAyd All green :) Right but the expected result doesn’t change depending on the argument, so this tests that pivot_table can accept the argument but doesn’t test that it actually ever makes a difference - does that make sense?

What I’m asking for is a test where observed=True would produce a different result than observed=False so we can be more explicit about this working. 

Sent from my iPhone

> On Mar 21, 2019, at 12:47 AM, Benjamin Rowell <notifications@github.com> wrote:
> 
> @benjaminr commented on this pull request.
> 
> In pandas/tests/reshape/test_pivot.py:
> 
> > @@ -65,6 +65,25 @@ def test_pivot_table(self):
>              index + [columns])['D'].agg(np.mean).unstack()
>          tm.assert_frame_equal(table, expected)
>  
> +    def test_pivot_table_categorical_observed(self, observed):
> +        # issue #24923
> +        df = pd.DataFrame({'col1': list('abcde'),
> +                           'col2': list('fghij'),
> +                           'col3': [1, 2, 3, 4, 5]})
> +
> +        df.col1 = df.col1.astype('category')
> +        df.col2 = df.col1.astype('category')
> +
> +        expected = df.pivot_table(index='col1', values='col3',
> As far as I'm aware, it makes use of the pytest fixture for observed.
> 
> When I execute the following test individually:
> 
> pytest -s -v pandas/tests/reshape/test_pivot.py
> 
> You see the test pass with all three scenarios.
> 
> pandas/tests/reshape/test_pivot.py::TestPivotTable::test_pivot_table_categorical_observed[True] PASSED pandas/tests/reshape/test_pivot.py::TestPivotTable::test_pivot_table_categorical_observed[False] PASSED pandas/tests/reshape/test_pivot.py::TestPivotTable::test_pivot_table_categorical_observed[None] PASSED
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
 > Right but the expected result doesn’t change depending on the argument, so this tests that pivot_table can accept the argument but doesn’t test that it actually ever makes a difference - does that make sense? What I’m asking for is a test where observed=True would produce a different result than observed=False so we can be more explicit about this working.
> […](#)
> Sent from my iPhone
> On Mar 21, 2019, at 12:47 AM, Benjamin Rowell ***@***.***> wrote: @benjaminr commented on this pull request. In pandas/tests/reshape/test_pivot.py: > @@ -65,6 +65,25 @@ def test_pivot_table(self): index + [columns])['D'].agg(np.mean).unstack() tm.assert_frame_equal(table, expected) + def test_pivot_table_categorical_observed(self, observed): + # issue #24923 + df = pd.DataFrame({'col1': list('abcde'), + 'col2': list('fghij'), + 'col3': [1, 2, 3, 4, 5]}) + + df.col1 = df.col1.astype('category') + df.col2 = df.col1.astype('category') + + expected = df.pivot_table(index='col1', values='col3', As far as I'm aware, it makes use of the pytest fixture for observed. When I execute the following test individually: pytest -s -v pandas/tests/reshape/test_pivot.py You see the test pass with all three scenarios. pandas/tests/reshape/test_pivot.py::TestPivotTable::test_pivot_table_categorical_observed[True] PASSED pandas/tests/reshape/test_pivot.py::TestPivotTable::test_pivot_table_categorical_observed[False] PASSED pandas/tests/reshape/test_pivot.py::TestPivotTable::test_pivot_table_categorical_observed[None] PASSED — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Yeah I see where you're coming from. So we need a test that benchmarks the two against each-other, as that's all observed should effect. @benjaminr I am not referring to an ASV. I am asking for an actual test where observed=False and observed=True would generate different results. Does that make sense or are we just not on the same page? > @benjaminr I am not referring to an ASV. I am asking for an actual test where observed=False and observed=True would generate different results. Does that make sense or are we just not on the same page?

I know what you're saying, but I'm afraid I can't think of a test that would achieve what you want.

Happy for someone else to look at it. @benjaminr ok so I could have the wrong expectation here. I took this example from the groupby tests:

https://github.com/pandas-dev/pandas/blob/6e0f9a971bcd07a4a6283dc03190b4363d42e292/pandas/tests/groupby/test_categorical.py#L262

```python
    cat1 = Categorical(["a", "a", "b", "b"],
                       categories=["a", "b", "z"], ordered=True)
    cat2 = Categorical(["c", "d", "c", "d"],
                       categories=["c", "d", "y"], ordered=True)
    df = DataFrame({"A": cat1, "B": cat2, "values": [1, 2, 3, 4]})
    df['C'] = ['foo', 'bar'] * 2
```

So pivoting off of that on master yields:

```python
In [8]: df.pivot_table(index='A', columns='B', values='values', aggfunc=np.sum)
Out[8]:
B  c  d
A
a  1  2
b  3  4
```

Which actually doesn't show any of the unobserved categories (in contrast to groupby behavior), so I don't think within the scope of this PR you could actually do anything to fix that.

So perhaps my hesitation here is that by accepting `observed` as a keyword argument to pivot we might be unintentionally signaling to users that they could get the cartesian production by passing `observed=False`, which actually isn't the case.

I see two solutions here:

1. Simply pass `observed=True` behind the scenes from pivot_table, removing it's exposure as a parameter OR
2. Having a follow up PR where `observed=False` would actually be supported through pivot_table

I'd prefer option 1 in this case but @jreback curious if you have any thoughts

 can you merge master