API: categorical grouping will no longer return the cartesian product
State: closed

closes #14942
closes #15217
closes #17594 
closes #8869

xref #8138



Labels:
name: Groupby
name: API Design
name: Categorical
description: Categorical Data Type

Comments:
this makes categorical groupers work like other groupers and should make things more performant and intuitive. It is somewhat walking back a change from when we first introduced categorical groupers. But the information is preserved (meaning the categories are still there), just removes the automatic re-indexing (which was causing memory to blow up).

still need some increased test coverage. note the first commit actually has almost all of the changes, the next are just cleaning up tests.
Just making sure, this affects grouping by a single categorical as well?
yes this affects just a single categorical column also 
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20583?src=pr&el=h1) Report
> Merging [#20583](https://codecov.io/gh/pandas-dev/pandas/pull/20583?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/28edd06498987972953b5c8e1c98b7f55d24b8d2?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `98.07%`.

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

```diff
@@            Coverage Diff             @@
##           master   #20583      +/-   ##
==========================================
+ Coverage   91.78%   91.79%   +<.01%     
==========================================
  Files         153      153              
  Lines       49341    49371      +30     
==========================================
+ Hits        45287    45319      +32     
+ Misses       4054     4052       -2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.18% <98.07%> (+0.01%)` | :arrow_up: |
| #single | `41.92% <5.76%> (-0.03%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20583?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/20583/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `95.94% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/20583/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.67% <100%> (+0.05%)` | :arrow_up: |
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/20583/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `92.62% <100%> (+0.07%)` | :arrow_up: |
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/20583/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `97.03% <100%> (ø)` | :arrow_up: |
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/20583/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `96.97% <87.5%> (ø)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/20583/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `84.59% <0%> (+0.2%)` | :arrow_up: |

------

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

so this is all ready to go if any comments. @TomAugspurger @jorisvandenbossche 


> yes this affects just a single categorical column also

That doesn't seem to be the case?

With this branch, and the example of the whatsnew docs:

```
In [14]: df.groupby(['A', 'B']).sum()
Out[14]: 
     values
A B        
a c       1
  d       2
b c       3
  d       4

In [15]: df.groupby('A').sum()
Out[15]: 
   values
A        
a       3
b       7
z       0               <---------- now unobserved category is included
```

Further a general comment (will try to do more detailed review later this week): I am not sure we can just change this. First, it is a API breaking change in several places, eg also pivot_table\*. And second, I think the current behaviour can actually useful in certain cases and it would be nice to have a way to keep this behaviour. 
I know this is very ugly, but it would be worth to have a keyword for this in `groupby` ?

\* but I agree we should look into it and try to make this more consistent. As, for example, `pivot` does not seem to include unobserved categories (already currently on master), while `pivot_table` does include them but apparently only for the index and not columns? (try `df.pivot_table('values', 'A', 'B')` with the example of whatsnew). On the other hand, `value_counts` does include them (and I think rightly so), but that also introduces an inconsistency with groupby.
So I could also change this for a single grouper. That breaks a couple of tests. I am inclined to do this actually as then it makes the multi and single case consistent. 

We cannot support a full cartesian product for multi-groupers as this will blow up memory and kill performance. So either we go for:

-  an option to turn this on/off. 
- differing behavior for single vs multi
- change single grouping behavior as well
I’d favor an option. I think the default should eventually be to exclude unobserved categories (after a deprecation cycle)

________________________________
From: Jeff Reback <notifications@github.com>
Sent: Thursday, April 12, 2018 5:39:16 AM
To: pandas-dev/pandas
Cc: Tom Augspurger; Mention
Subject: Re: [pandas-dev/pandas] API: categorical grouping will no longer return the cartesian product (#20583)


So I could also change this for a single grouper. That breaks a couple of tests. I am inclined to do this actually as then it makes the multi and single case consistent.

We cannot support a full cartesian product for multi-groupers as this will blow up memory and kill performance. So either we go for:

  *   an option to turn this on/off.
  *   differing behavior for single vs multi
  *   change single grouping behavior as well

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/20583#issuecomment-380759167>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHItJEfaXLGyuztAiN7hpsUYEzKUqRks5tny7UgaJpZM4TDk6b>.

this is read for a look

@TomAugspurger @jorisvandenbossche 
http://pandas-docs.github.io/pandas-docs-travis/categorical.html#operations would be a good place to show `observed=False`.

About `pivot_table`, I always forget what dropna does... Does it control dropping columns / rows that are all NA before or after aggregating?
I will try to give this a more detailed look tomorrow.

But general comment: now that we have the keyword to specify the behaviour (for now with `observed=False` for back compat), I am not really sure that we should change the default in the future.

It's difficult to judge, as I personally don't run much in such situations, but my gut feeling says that the original issue that motivated the change (the combinatorial explosion with multiple categorical groupers) is not the majority usage pattern of categoricals. And if you don't want to include unobserved categoricals in your analyses in general (groupby, pivot, value_counts, plotting, ..), you always have the easy functionality of `remove_unused_categories`.

cc @jankatins 
@TomAugspurger the pivot was a red herring, was not passing things thru. and ``dropna`` (in pivot) is exactly equivalent of the ``observed`` kwarg (to groupby). So maybe should just rename ``observed`` -> ``dropna``, and would be consistent across other functions (``value_counts``) as well.

@jorisvandenbossche I disagree. You almost *never* want a cartesian product of all of the groupers. It can easily blow you up and shouldn't be the default. (its also easy to create if you need). Not this has *nothing* to do with the actual categories that are returned, they are in BOTH cases indicated on the dtype of the level of that index, (observed or not), its the groupers that are the issue.
@jorisvandenbossche I also think the default should change. Categoricals are mostly an implementation detail, and should behave as similarly as possible as ordinary ``Series``.
> You almost *never* want a cartesian product of all of the groupers. It can easily blow you up and shouldn't be the default. 

But in most cases you have *no* cartesian product, you only have a single categorical key

> It can easily blow you up and shouldn't be the default. (its also easy to create if you need).

It's also easy to get the version with only the observed ones (certainly now there is the keyword), so that is not really an argument IMO

> Not this has nothing to do with the actual categories that are returned, they are in BOTH cases indicated on the dtype of the level of that index

Yes, and that is certainly a good thing. But for me it is still the question what we want the *visible* output to be. 
For example a pivot table is often used as a kind of a summary table. In such a case, I often do want to know that a certain category I care about (otherwise it would not be in the categories) has no values. 
And it is also about consistency: `value_counts` does include the unobserved one (and rightly so, IMO).
My original motivation to work on categoricals were stuff like lickert scales ("completly agree ... completly disagree", 5 to 7 values). For that seing unobserved categoricals in group bys are a good thing.

Since then a lot of changes have been made to make categoricals usefull in other situations (like as a memory efficient string replacement or to analyse genes?).

This change feels like it is a change to make working with the latter easier but will make the former harder.
> And it is also about consistency: value_counts does include the unobserved one (and rightly so, IMO).

I gave it as granted that if we change ``groupby`` we will also change ``value_counts`` - otherwise I agree it doesn't make any sense.

> This change feels like it is a change to make working with the latter easier but will make the former harder.

True... I just think your use case is more rare (but I have no data to support this statement, other than personal experience). Anyway, with the new argument both things will be pretty easy anyway, it is mainly a matter of which use case should require awareness from the user.
You can indeed use Categorical as memory-efficient string storage if you have some repetition, but, I don't think we should take that use case into account for designing the *default* API (note I say default, it's certainly we make that use case *possible*, which it is with the keyword).

Of course, you can still have "real" categorical data with many categories, from which you typically only observe a subset. And then you might want to have the dropping behaviour as default. 
My feeling says that the use case with fewer categories of which you typically observe the majority is more common, but I also have completely no objective idea about this :-)
agree for consistency should change:

```
In [1]:  pd.Series(list('aabc')).astype(pd.api.types.CategoricalDtype(list('abcd'))).value_counts(dropna=True)
Out[1]: 
a    2
c    1
b    1
d    0
dtype: int64

In [2]:  pd.Series(list('aabc')).astype(pd.api.types.CategoricalDtype(list('abcd'))).value_counts(dropna=False)
Out[2]: 
a    2
c    1
b    1
d    0
dtype: int64
```

so that ``True`` would remove the unobserved.
I think in the long run I'd prefer unobserved categories to be excluded from groupby, primarily because the cartesian product issue seems like such a difficult gotcha. Though if / when we have an "interned string" array type (categorical, but with different semantics), would we want to go back on that change?

---

`value_counts` highlights my discomfort with overloading `dropna`. It will require an `observed` keyword to handle cases like

```python
In [21]: c1 = pd.Categorical(['a', 'a', None], categories=['a'])

In [22]: c2 = pd.Categorical(['a', 'a', None], categories=['a', 'b'])
```

I don't think `c1.value_counts(dropna=True)`  should be the same as `c2.value_counts(dropna=True)`. `c2` has the additional category. So either recommend `c2.remove_unused_categories.value_counts(dropna=True)` or `c2.value_counts(dropna=True, observed=True)`.
Might be a crazy idea... but just in case it's not, I'll rather propose it now than later: what if showing empty categories (or not) was a property of the _dtype_, preserved across transformations, just like the (possible) ordering?

That way, @jankatins would need to specify only once, when creating the categorical for the first time, that his categories (the Likert scale values) are "all important". Otherwise, the categories would be considered as merely "functional".

We could even think that a categorical created by passing the categories has "important" categories by default, while a categorical which creates its own categories automatically has "functional" categories by default (clearly with a parameter allowing to change this). (But this might be too much magic.)
I would rather make a "dict-encoded / interned string" type than overloading Categorical with this. Because in such a case, you would typically also not care about introducing new categories (eg when concatting etc), which now raises errors for Categorical. We have had discussion about exactly that before: https://github.com/pandas-dev/pandas/issues/8640 (but you will see the discussion is also a bit dated :-))
In a pandas with an interned string array, would we want the `observed` keyword? I think so, but the default would stay `False`, as it is now.
> I would rather make a "dict-encoded / interned string" type than overloading Categorical with this

While I have no objection to relying on a different (as in "differently named") type, which in turn might benefit from convenience functions to categorize strings (thanks for the link @jorisvandenbossche ), it seems to me that 1) there is no need to make that different dtype specifically string-ish 2) the implementation would/could be 99% identical to ``Categorical`` 3) it would be useful to be able to switch from one type to the other without re-encoding, and 4) anyway, the fact that we don't allow categories to change is a limitation of implementation, not a virtue, hopefully to be lifted some day regardless of this issue (am I wrong?)

In particular, if 3) was true (which would most certailny be the case if 2) was true), then the use of the ``observed`` keyword would be quite redundant ( @TomAugspurger ). (Which is not to say it is necessarily useless - it is mostly a matter of which is the most convenient API: what do you think @jankatins ?)
> 4) anyway, the fact that we don't allow categories to change is a limitation of implementation, not a virtue, hopefully to be lifted some day regardless of this issue (am I wrong?)

As far as I know, this is by design (to preserve the integrity of the categorical dtype). We have eg a `union_categoricals` helper function if you want that when merging data: https://pandas.pydata.org/pandas-docs/stable/categorical.html#unioning (I seem to recall we had quite some discussion if we should use that by default in `concat` or not)

> In a pandas with an interned string array, would we want the observed keyword? I think so, but the default would stay False, as it is now.

That's a bit what I meant above with that I think we should design the Categorical default behaviour for the "categorical" use case, and not for the "memory efficient strings" usecase for which it is also used in absence of a better string dtype. 
> General question (in case we keep the deprecation warning): do we want to show it always? Or only in the case where you actually have unobserved values?

we *only* show it if you have a categorical in the groupers. I think this is a fine case to surface to the user to make them be explicit. Otherwise you end up showing it depending on the data, generally not a great idea.
updated, but didn't address @jorisvandenbossche comments yet.
@jreback what are your thoughts on the likelihood of an "interned string" extension array (for 0.24)? It'll be able to share much of the implementation with Categorical, but would have different semantics around groupby, value_counts, pivot_table, etc.

Assuming we had that already, would `observed=True` be a good default *for Categorical* (it wouldn't apply to the interrned string EA)?

Since we don't have that extension array today, I think this PR is still great to have for 0.23. I'm just worried about the default being `observed=True`.
well I think we *could* do a String dtype, ideally would be based on ``pyarrow`` but don't think that will be ready, we *could* store as a fixed length numpy array with offsets so that might work. I think this will be some work to do it though (was planning on int NA better)
> Since we don't have that extension array today, I think this PR is still great to have for 0.23. I'm just worried about the default being observed=True.

I think even IF we have a string type (EA), this should still be ``observed=True``. The reason is that this can blow up a grouper very easily (or work, yet be very very slow).
(agreed about int NA being more useful, but let's keep that separate).

I could see a use-case for an general String EA, and an InternedString EA
(or DictEncodedString). What you're describing sounds like the general
String type.

InternedString would have the same impl as Categorical: a set of codes
indexing into a Index of strings (instead of categories). But things like
`_codes_for_groupby` would be different, since it would exclude unobserved
categories.

On Mon, Apr 30, 2018 at 8:30 AM, Jeff Reback <notifications@github.com>
wrote:

> well I think we *could* do a String dtype, ideally would be based on
> pyarrow but don't think that will be ready, we *could* store as a fixed
> length numpy array with offsets so that might work. I think this will be
> some work to do it though (was planning on int NA better)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20583#issuecomment-385398333>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIr4_-tfHZS01jrR1bwQEyoy_ZdX5ks5ttxHYgaJpZM4TDk6b>
> .
>