Deprecate groupby/pivot observed=False default
State: closed

- [x] Relates to #17594, Closes #30552
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Had a relatively small 70k data frame that I was trying to do a groupby sum on blow up on me today. This was the reason. I had something like zip codes and cities as categoricals, expected SQL-like groupby but instead got a cartesian product of 'cities' and 'zips'. Sounds like there was some previous desire to explore a new default.

Didn't try to do any wild stuff to keep up with the stacklevel depending on where this was called from.

Labels:
name: Categorical
description: Categorical Data Type
name: Deprecate
description: Functionality to remove in pandas
name: Needs Discussion
description: Requires discussion from core team before further action
name: Stale

Comments:
hmm i thought we had an issue about 
Will see about avoiding `pytest.warns` though the CI stuff doesn't make it real clear why. I think the included warnings assertions didn't like asserting there wasn't a warning but I'll check again. 

First question is, do you want this PR? Does it need discussion?

Second question is, what do y'all suggest I do about the warnings coming in the examples? Suppress them? Fix the examples to use use the new keyword?
we don't use pytest.warn instead use tm.assert_produces_warning

i think the intent of the PR is good - haven't looked closely yet. 
https://github.com/pandas-dev/pandas/issues/30552 is the related issue.

I'm unsure about how to proceed here. We've overloaded `Categorical` for two purposes: 1: a statistical concept for the fixed set of categories, and 2: the memory-savings for low-cardinality data. I think that `observed=True` is a good default for Categoricals and a bad default for memory-saving. I think my ideal outcome is to

1. implement a dtype that just does the dictionary encoding (https://github.com/pandas-dev/pandas/issues/20899).
2. Change the default `observed=False` to `observed=None` to mean "ask the extension type for its default." Then `Categorical` can implement `observed=False` by default and the Dictionary-encoded type can have `observed=True`.

But that's also much more work that this PR, so like I said, I'm unsure how to proceed.
Yeah, that makes sense as a better solution, and I was of two minds about whether to do this, but I struggled to find an example for when I'd ever want `observed=False`. Do you have one in mind? (**Edit**: see your example of survey responses. Let me think through that.). If I really wanted a cartesian product of things I don't actually have in my data, I don't _think_ I'd reach for groupby to do that.

In the above example, I think I'd just never use Categoricals but I'd want all of their sugar for my Dictionary-encoded type.
I started fixing up the doc warnings, but I think there's some more I need to think through with crosstab and `dropna`.
Yeah, this is basically my position.

https://github.com/pandas-dev/pandas/issues/30552#issuecomment-602849215
Rebased to get rid of the merge conflict. Not sure why the coverage tests are saying I added untested lines.
Going to be a bit of (tedious) work to get the tests and doc builds passing on the warnings as errors runs, let me know whether this is likely to get merged, and I'll come back and fix the tests and look at the crosstab stuff.
I'd say something like 50-80% probability of being accepted? As you say, it's only sometimes where this behavior is desired for (statistical) categorical columns, and it's never desired for the memory-savings purpose.

cc @jankatins, since I suspect this goes back to the original categorical implementation, if you have thoughts.
Yes, the original usecase I had in mind was a survey with lots of likert like scales: "Strong Disagree ... neutral ... Strong Agree". The original categorical was also build around what R does for factors. All the "problems" started o surface when someone discovered that categoricals save memory and time when dealing with strings. 

The basic group by was "aggregator(num_col) per cat_column_y" and it should produce the same structure (ordering,number of rows) no matter if cat_column_y contained all values or not (so NA/0), e.g. to get nice plots which look structural similar in a report. For the same reason I would guess it was decided that group bys with two cat columns should show all combinations. 

Categoricals defaults are (or at least were at the beginning) all geared towards that usecase. If stuff like this (and there were already others) take over it makes sense to simply rename it to something like "DictEncodedArrayBase" and add a new Categorical and a "StringDictEncodedArray" on top of it... :-)
> All the "problems" started o surface when someone discovered that categoricals save memory and time when dealing with strings.

Just want to gently push back on the (maybe perceived) notion that this is somehow an abuse of categoricals. High-cardinality, non-independent categoricals/factors are definitely a thing, and the default can not only explode memory but also gives non-sensical answers.

I think the departure from (the flavors of) SQL(s I use) was more unexpected for me.

Buut, as I mentioned in the issue thread, I've definitely wanted both behaviors of this just today. Is there some design philosophy here to fallback to for guidance (like prefer standard SQL semantics or refuse the temptation to guess or something)? I'm not sure just a type with sensible defaults is going to solve the issue for me. E.g., I want the `observed=True` behavior for two independent factors but not for two dependent factors and only when they'd both be included in a groupby. It's the presence of another one that leads me to want the behavior.

All this said, now that I know and am thinking about these things I think I'll always have to specify observed. Part of me just wants to raise an error if `observed=None` and force folks to think, but that'd be a pretty bad UX.
For me this feels a bit like the "stringsAsFactors" saga on R: https://simplystatistics.org/2015/07/24/stringsasfactors-an-unauthorized-biography/ :-)
> For me this feels a bit like the "stringsAsFactors" saga on R

Ha, indeed. Now you've got me rethinking everything.
Been thinking about this a bit, given the comments and use cases. I have a proposal that may or may not be a good one. 

What about making the default None and in the presence of a categorical, if the default isn't changed it raises an error with a message that makes users choose True or False. 

This would be noisy but would avoid the temptation to guess and whatever "my way is the most typical" bias that could creep in. It wouldn't force a "stringsAsFactors" situaish just an "ugh SettingWithCopyWarning" situaish (which I can live with). It also could be temporary until there's another extension dtype ready that's more appropriate for situations where your strings aren't factors (or your factors aren't independent). 

Thoughts?
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.
>  What about making the default None and in the presence of a categorical, if the default isn't changed it raises an error with a message that makes users choose True or False.

That sounds fine to me (with the default `None` being a warning for now, saying that an exception will be raised in the future).
@jseabold yeah sorry haven't gotten to this, but if you can merge master and implement @TomAugspurger suggestion would be great.
Yeah, sounds good.
@jseabold if you have a chance to merge master and fix this up
```
Traceback (most recent call last):
  File "/home/runner/work/pandas/pandas/scripts/check_for_inconsistent_pandas_namespace.py", line 59, in <module>
    main()
  File "/home/runner/work/pandas/pandas/scripts/check_for_inconsistent_pandas_namespace.py", line 49, in main
    raise AssertionError(
AssertionError: Found both `pd.DataFrame` and `DataFrame` in pandas/tests/groupby/test_grouping.py
```

lol. Come on!

I rebased on master. One thing this brought to my attention is that there's not a consistent way to choose the behavior you want for methods that rely on the underlying code. E.g., `Series.value_counts`, `DataFrame.sum`, etc.

Cleaning up the docs, I'm also not sure that `pivot_table` does what I expect it do in master. This is master. Wouldn't you expect there to be a c for A in the below? Regardless this PR retains the behavior in master. It now warns but the observed keyword doesn't appear to actually do anything unless I'm missing something.

```python
In [1]: paste
    raw_cat = pd.Categorical(["a", "a", "b", "b"], categories=["a", "b", "c"])
    df = pd.DataFrame({"A": raw_cat, "B": ["c", "d", "c", "d"], "values": [1, 2, 3, 4]})
    pd.pivot_table(df, values="values", index=["A", "B"])
## -- End pasted text --
Out[1]:
     values
A B
a c       1
  d       2
b c       3
  d       4

In [2]: pd.pivot_table(df, values='values', index=["A", "B"], observed=True)
Out[2]:
     values
A B
a c       1
  d       2
b c       3
  d       4

In [3]: pd.pivot_table(df, values='values', index=["A", "B"], observed=False)
Out[3]:
     values
A B
a c       1
  d       2
b c       3
  d       4
```
> It now warns but the observed keyword doesn't appear to actually do anything unless I'm missing something.

Oh, you're supposed to use `dropna` for this and the keywords interact. I guess the warning isn't general enough given that the keywords that control this behavior are different depending on entrypoint. The observed keyword here only matters if dropna is False.

Is there ever a case where dropna is True and observed should be False, or can we just always set observed to False and avoid the warning in the default case?
> Oh, you're supposed to use dropna for this and the keywords interact. I guess the warning isn't general enough given that the keywords that control this behavior are different depending on entrypoint. The observed keyword here only matters if dropna is False.
>
> Is there ever a case where dropna is True and observed should be False, or can we just always set observed to False and avoid the warning in the default case?

Answering my own question. Somewhere along the line observed=False stopped returning NaNs and started returning 0s. Didn't see when this happened glancing at the release notes between [v0.23.0](https://pandas.pydata.org/docs/whatsnew/v0.23.0.html#new-observed-keyword-for-excluding-unobserved-categories-in-groupby) and master.
Hello @jseabold! 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 2020-12-07 19:55:34 UTC
Finished the warnings whack-a-mole. Silencing warnings in years old what's new docs seemed like not the right thing to do, but it's in there. I don't know if you re-publish these and/or accept ipython directives in them, but if so they're likely not historically correct anymore.
cc @jbrockmendel @jorisvandenbossche @TomAugspurger if any objections.
> Finished the warnings whack-a-mole. Silencing warnings in years old what's new docs seemed like not the right thing to do, but it's in there. I don't know if you re-publish these and/or accept ipython directives in them, but if so they're likely not historically correct anymore.

yeah for old docs silencing is ok (or replacing with a code-block but); for the docs themselves we like to fix it (e.g. being explicit).
> can you update the tests so we filter less

The other alternative where it's not possible to hard-code observed is to add this everywhere, which is what I did at first. Seemed like the filtering pattern was used more elsewhere.

```
with tm.assert_produces_warning(FutureWarning if observed is None else None, ...)
    ...
``` 
internal use of groupby need to hardcode True (and maybe reindex if needed)
> internal use of groupby need to hardcode True (and maybe reindex if needed)

This would be a breaking change. The default now is False. Is that what you mean? Then every internal use of groupby will need a separate deprecation warning unless we leave them all as None (as they were) and catch the warnings. That's why I caught the warnings in the places where the user can't actually do anything but a future change will change this behavior. Really, everything that depends on the current behavior needs a new keyword so the users can ask for it or not, but I'm not sure I have that in me tbh.