BUG: Categorical.unique should keep dtype unchanged
State: closed

- [x] closes #18291
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

We want to keep the same dtype as in the original after applying `unique`. For example:

```python
>>> dtype = pd.CategoricalDtype(['very bad', 'bad', 'neutral',  'good', 'very good'], ordered=True)
>>> cat = pd.Categorical(['good','good', 'bad', 'bad'], dtype=dtype)
>>> cat
['good', 'good', 'bad', 'bad']
Categories (5, object): ['very bad' < 'bad' < 'neutral' < 'good' < 'very good']
>>> cat.unique()
['good', 'bad']
Categories (5, object): ['bad' < 'good']
>>> cat.unique().dtype == cat.dtype
False  # master
True  # this PR
```


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

Comments:
Any comments, @jreback , @jbrockmendel & @gfyoung ?
cc @jorisvandenbossche @TomAugspurger any objections. I actually think we should include this in the RC, cc @simonjayhawkins 
cc @toobaz from the OP
Updated.
> I am fully on board with the actual change (it's really strange behaviour), but I think we need to think a bit more about _how_ to do this change. Because it's not a bug fix, this was intentional, documented behaviour. And especially in the case of an ordered categorical, it's potentially breaking.
> So IMO we don't need to "hurry" it into 1.2 (it's a longstanding issue, I don't think there is a reason why it is now pressing or blocking for 1.2?). We can also discuss a bit more and merge it early in the 1.3 cycle.

I don't think its possible to actually deprecate this. Further this is on an actual Categorical *only*. I honestly don't think this is that big of a deal. am ok with releasing it here with a sub-section.
> Further this is on an actual Categorical *only*.

No, the Series unique method dispatches to the underlying EA, so Categorical.unique in this case, so changing this also affects the Series behaviour.


I see @topper-123 you did have to change a couple of series tests.

@jorisvandenbossche how would you deprecate this behavior? 
I think the current behaviour is buggy behaviour, because users wouldn't expect `arr.unique` to change dtype (even though it's documented here, as you also say). I think the current behaviour stems from before we had categorial dtypes, so the concept of dtype equality for categoricals didn't really exist back.

A deprecation cycle would require a extra parameter on both `Series`, `Categorical` and `CategoricalIndex`. Can we postpone this to 1.3, but treat is as a bug? That would give potential downstream problems more time to be fixed.

@jorisvandenbossche unless you can conceive of a good way to cleanly deprecate this I am +1 to merging here. I agree its a change, but the prior behavior was just grandfathered in w/o consideration. This makes me ok with just changing it.
> I think the current behaviour stems from before we had categorial dtypes

This specific `unique` behaviour was explicitly implemented for categorical, I think: https://github.com/pandas-dev/pandas/pull/10508

But anyway, it's indeed true that it's not that easy to deprecate .. As @topper-123 says, it would require an extra keyword in the different `unique` methods, specifically for this, and then afterwards we will also want to get rid of that keyword again ..  
The other alternative is to keep it as a breaking change for 2.0.

I actually first thought this also affected the order of the actual returned values (which would be a much bigger change), but apparently it's only the order of the categories of the returned categorical (in addition to dropping the unobserved ones). So in that case, I am fine treating it as a bug fix. But still a slight preference to keep it for 1.3.

For people wanting the old behaviour, we should probably point to `remove_unused_categories`

> > I think the current behaviour stems from before we had categorial dtypes
> 
> This specific `unique` behaviour was explicitly implemented for categorical, I think: #10508
> 
> But anyway, it's indeed true that it's not that easy to deprecate .. As @topper-123 says, it would require an extra keyword in the different `unique` methods, specifically for this, and then afterwards we will also want to get rid of that keyword again ..
> The other alternative is to keep it as a breaking change for 2.0.
> 
> I actually first thought this also affected the order of the actual returned values (which would be a much bigger change), but apparently it's only the order of the categories of the returned categorical (in addition to dropping the unobserved ones). So in that case, I am fine treating it as a bug fix. But still a slight preference to keep it for 1.3.
> 
> For people wanting the old behaviour, we should probably point to `remove_unused_categories`

ok sounds good, repurposed to 1.3
Ok, I'll update this PR so it's part of 1.3. It'll be in a couple of days, as I don't have so much time right now.
Updated.
Updated.
> If we do the change for unique, we should probably follow that new behaviour in groupby?

Yes, I agree. I can take it in a follow-up. I think we should use a deprecation there, but I can open an issue to discuss that.
Is this ok? The travis failure is unrelated.
> I think we should use a deprecation there

How would you deprecate this? 
(if know I argued above for a deprecation for unique first as well, but in the end we decided not to do that since there is no clear way how to do this. I think the same applies for the groupby case?)




> How would you deprecate this?
> (if know I argued above for a deprecation for unique first as well, but in the end we decided not to do that since there is no clear way how to do this. I think the same applies for the groupby case?)

That's probably right. An idea could be a setting in `pd.options` for users who want to revert to the old behaviour (e.g. if tests are failing).

If the above is not ok, an idea is the non-clean approach: a parameter in the `.groupby` method (`drop_unused_categories`?) will be set to `None` by default. A deprecation warning will be emitted, if it's not set to True or False. `drop_unused_categories=None` will be interpreted as `drop_unused_categories=True` until pandas 2.0.
In pandas 2.0 `drop_unused_categories=None` will be interpreted as `drop_unused_categories=False` and setting `drop_unused_categories` to either True or False will emit a deprecation warning. In pandas 3.0, that parameter will be removed.

The downside to the above solution is that all users will have to set the new parameter, which will annoy users and the deprecation process is very slow.
Bu such a `drop_unused_categories` keyword could be exactly the same for `unique`, right? (it's not specific to groupby) 
So if we consider it for groupby, we should do the same for unique.

-1 on deprecating

let's just change it
Deprecating would also mean different dtype after applying .unique, which is wrong behaviour.

I`m ok with just changing, like @jreback.
To be clear, I was not arguing that it should be deprecated. I was mainly arguing for 1) if we change it for unique, we should do the same change for groupby, and 2) if we agree on that, I would simply do it here in this PR (because the second is simply a consequence of changing the first, and now you actually *added* code to not have this change in the groupby case. So fixing it here means that this doesn't need to be added in the first place)
i dont have an opinion on the groupby thing, but i would like to see the Categorical.unique fix get merged.  @topper-123 what're your thoughts?
> cc @toobaz from the OP

Sorry for the slow reaction. I don't understand the discussion on deprecating the ``groupby`` through some ``drop_unused_categories``, as unused categories are _already_ kept in ``groupby`` operations, right? So the only problem is the sorting behavior (of the dtype's categories) that would change.

And unless I missed something, anybody who relied on sorted categories loosing their sortedness in any way during a ``groupby`` operation was doing something so weird and buggy that we shouldn't bother too much.

So _if_ doing it in this PR really amounts to removing that code block, I think that's a good idea. But there will presumably be many tests to fix, so I also find reasonable to do it in a separate PR.
I`ve rebased. I can look into the groupby thing in the weekend, if this can not be merged seperately.
@topper-123 tests.indexes.test_common.TestCommon.test_unique i think can be updated to not-skip CategoricalIndex
Thanks, @jbrockmendel , I've changed that.

I've worked on keeping the dtype unchanged in groupbys also, but it's quite difficult with the various combinations of `sort` and `observed`. I find the groupby mechanism in general quite difficult to follow, as there seems to be a high degree of branching going on. Any suggestions on how to proceed?

Alternatively, I'd like this PR to be just merged and work on the groupby later/seperately, if that's ok. Limiting this dtype issue to groupbys is still a nice progress overall without downsides IMO.
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.
I can't figure out to to get the groupby to pass in all four groupby cases with these four parameter values:
* sort=False, observed=False
* sort=True, observed=False 
* sort=False, observed=True
* sort=True, observed=True

Is it possible to either:
* merge this as is (fixes only the `Categorical.unique` issue)
* Someone else fixes the groupby issue
?
> merge this as is (fixes only the Categorical.unique issue)

i think this is worthwhile