BUG: Fix regression for groupby.indices in case of unused categories

[phofl]

• closes REGR: GroupBy.indices no longer includes unobserved categories #38642
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

To show the unused we categories, we can dispatch back as we did before #36842. Since Categorical does not support nan, the reason for the switch does not exist in this case. I think handling unused categories in get_indexer_dict is not desirable, because it would introduce a lot of convolution for one corner case we could handle here pretty easily.
Since we are only using indices in grouper only for categoricals, we do not have to worry about sorting as the test shows.

cc @jorisvandenbossche @mroeschke

[jreback]

woa, this is so special casing here. why is this added?

[jorisvandenbossche]

See the explanation in the top post

[phofl]

thx

[jreback]

I read it and don't buy it. Why are we supporting this case at all? I would rather simply fix this breaking or not.

[phofl]

You mean not showing unused categories for indices?

[jreback]

i c we did thave this special case before, but onl was for len(self.groupins) == 1. its the special isinstance i object.

[jorisvandenbossche]

Since Categorical does not support nan, the reason for the switch does not exist in this case.

@phofl Categorical does actually support NaNs, but maybe this is not covered by the tests?

[jorisvandenbossche]

It seems this might actually also be broken for Categorical (or not fixed by #36842). This is what I see on master:

In [8]: df = pd.DataFrame({'key': pd.Categorical(['a', 'b', 'a', 'b', np.nan]), 'col': range(5)})

In [9]: df.groupby('key', dropna=False).indices
Out[9]: {'b': array([1, 3]), 'a': array([0, 2])}

In [10]: df.astype({"key": object}).groupby('key', dropna=False).indices
Out[10]: {'a': array([0, 2]), 'b': array([1, 3]), nan: array([4])}

[phofl]

Hm, I misunderstood #35646 (comment) this then. Have to look into this then

[jorisvandenbossche]

Ah, no, I was not aware of that categorical with dropna=False might be knowingly not supported at the moment.
cc @mroeschke is there a reason that we wouldn't want to support that?

BTW, if this is the case, the docs of the dropna keyword should mention this.

[jreback]

i c we did thave this special case before, but onl was for len(self.groupins) == 1. its the special isinstance i object.

[mroeschke]

My comment was assuming that that adding a new dropna keyword argument to Categorical indicating that nan shouldn't be a missing value but a category itself wouldn't be a popular change. That assumption might be incorrect though

[jreback]

My comment was assuming that that adding a new dropna keyword argument to Categorical indicating that nan shouldn't be a missing value but a category itself wouldn't be a popular change. That assumption might be incorrect though

not happening, we already removed this case thru a deprecation cycle and no reason to add it back

[jorisvandenbossche]

Categorical indeed does not support NaN as a category, but does that mean it could not be supported in groupby(dropna=False)? A categorical can have missing values, so the resulting index of groupby(dropna=False) would not have NaN as category, but as missing value (as -1 in the codes)

[mroeschke]

I imagine it should be supported in groupby(dropna=False).

At the time I was probably focusing too much on Grouper.indices unaware that there was also BaseGrouper.indices (and the relationship between them) because Grouper.indices was relevant for a groupby rolling bug I was hunting down.

[jreback]

@phofl can you merge master and add a note for 1.2.1

[phofl]

Done

[jreback]

thanks @phofl very nice

[jreback]

@meeseeksdev backport 1.2.x