Fixups for categoricals
State: closed

This PR will collect the various fixups for Categorical.

See #8078 for the ToDos.

Closes: #8077 -> API/FIX: wrap result cut/qut in Series and make it ordered
Closes: #8074 -> Discussion
Closes: #8078 -> Todos

This has a (passing) unittest for #8138 but no code changes. Not sure what was the change which fixed the bug there...


Labels:
name: Categorical
description: Categorical Data Type

Comments:
@JanSchulz Thanks for working on this! But, maybe it is simpler that you just put different PRs instead of one big. Then we can also more quickly merge if one part is ready, not waiting on other issues. There is now a clear to do list, and some of the issues are not necessarily dependent on each other. Eg I would leave this PR just as a PR to fix `cut/qcut`

I think that's fine with this change but the rest will become dependend on each other. I will add a new PR for the rest but would like to do the rest in one PR (I will do one commit per change, hope that helps)

OK, do as you seem fit, no problem! (but this can indeed go in as a seperate PR)

@JanSchulz some feedback on the `ENH: let Categorical.reorder_levels also remove levels` commit. I think that is a nice feature, but I don't know I would use that as default. In a sense that, if I only want to 'reorder' the levels (the 'core' functionality of the function), it would be nice that the function warns me when I eg forgot a level. Now it will silently drop those. 

So just spawning ideas:
- have a `strict` keyword (or other name), that if `True` -> only same length is allowed, and if `False` -> you can also add/remove items in one go. If this would be True by default, then such removing of levels by accident cannot happen, as the user should set this to False to specifically say: 'Yes, I want to add/remove levels and I am myself responsible to check that I provided the correct items'. The error message when this kwarg is True could also point to set it to False.
- have seperate functions like `add_levels`, `remove_levels` and keep `reorder_levels` as is. Or maybe these would be useful as such, regardless of this discussion. Because, if you just want to add or remove a level (without reordering in the same time), this could be simpler as using `reorder_levels` where you also have to specify the others.

I like the `strict` idea. Will amend that commit.

for the name `strict`, @jreback do you know of other places in pandas where a similar functionality is used? (to have consistent naming in that case)

I also think my second bullet (the add_levels/remove_levels function) could also be useful irregardless of the behaviour of `reorder_levels`

`strict` is ok, not really sure its used elsewhere. I think you'd rather have the nomeclature of set/reorder indexes though?

e.g.

`reorder_levels` (change the ordering, must have same number of levels)
`set_levels` (can say any and can actually change the ordering / no check on number of levels)
`reset_levels` (I guess this is remove)?

@jreback @jorisvandenbossche re Strict: There are actually 4 types of actions you can make:
1. rename levels (currently done by assigning new things to `levels`)
2. removal of levels (can be done by `reorder_levels` on any place in the levels and by assignments to `levels` at the last places)
3. additions of new levels (can also be done by both: at any place by `reorder_levels` and after the last place by assignments to `levels`
4. reorder current levels (by `reorder_levels`)

So, the "strict" idea is actually very good, but if there should be a operation per "action", then rename, remove and reorder can be done without "sideeffects", but additions not: should that take a list of only the new levels (and put that at the last/first place in the list?) or should that take a list of all levels and it's then a kind of `reorder_levels`, because it can insert at any place?

One "not nice" thing is, that each of this sideeffect free operation would actually take quite a lot of checks which make it saver but are actually unnecessary if we would allow them...

@JanSchulz how's this coming?

I will work on this over the weekend. Sorry for the delay...

I will take the "strict" path for reorder. I really don't like the idea of adding a method per action (remove, add, reorder, rename) which in the end would all be some kind of "prevent side effects"-checks and then call some kind of method which would be like the current `reorder_levels()`. So the current end result is:
- `reorder_levels( ... strict=True)` -> can only reorder levels, will check that the set of new levels are the same as the set of current levels
- `reorder_levels( ... strict=False)` -> can reorder levels,  including adding new ones (will be unused) or remove some (will become `-1` = NaN)
- assigning to `levels` -> renames levels, can be used to remove/add levels from the end of the list.

I'm actually considering to make assigning to levels stricter, so that it only can rename, without the possibility to add/remove levels.

Anny comments?

I have the feeling that the `add/remove/reorder_levels` needs some more discussion. So maybe remove that one commit from this PR? (the one to last commit 'ENH: let Categorical.reorder_levels also remove levels … ') 
So we can review/try to get the rest of this PR merged as soon as possible, and the we can discuss it further somewhere else (I can create a new issue).

I can put that at the end, but I would really like to get the rename (levels -> categories) done in this PR and this will touch everything...

OK, the possible addition of other methods like `add/remove_levels` is maybe complementary to this and this does not exclude that, so maybe not a problem. (although I still think adding seperate methods can be useful, but lets keep that for somewhere else)

+1 on assigning to `levels` be strict (so check for length, only rename)

Just to make the discussion a bit clearer: There are 4 "actions": rename, reorder, remove and add levels.
- removing: remove the levels from the list (=Index in this implementation) and then refactorize to get the codes redone
- reordering: check that all items from old are in new and then redo the factorize step
- adding: check that all old are in new and then do a refactorize 
- renaming: simple replace the current list with a new list

So only the rename is a step without a factorize (which is probably the most expensive step in all actions) and IMO it makes sense to have these three actions working under one command so that would not need to do three factorize to add, remove and then reorder levels.

What could be done is add convenience methods which do the additional checks and then call `reorder_levels(..., strict=False)` or something like `manipulate_curent_levels(...)`.

-1 on doing this all in one method. Why are we against: `rename/reorder/add/remove` ?
this is how it is on all other objects.

@jreback: because then we have three `factorize` steps, which could be one (if one needs to do all three of remove, add and reorder, which I think is not uncommon, now we advertise the use `s.astype("category")` way, which produces "unordered" levels with missing "unused" levels in most cases and removal of "99" or "-" levels are also not uncommon). 

API is much more important that the (very minor) perf impact here. And you can always assign to levels (and/or create a new Categorical if you really care of that).

An idea:
- rename current `reorder_levels` to `change_levels(new_levels, rename=False)` -> if `rename==True` do a rename, otheriese the old codepath.
- add seperate `add/remove/reorder/rename` functions, which do the checks and then call `change_levels(...)`
- expose all  + assigning to `levels` under `Series.cat`

-> If one wants the power methods, use `change_levels` and for single changes use the appropriate named specific method.

This was the reason I proposed to move this to a seperate issue/PR ... :-) Because we would start to discuss again (which is good).

Maybe change_levels -> set_levels? which can be see as the more powerful equivalent of setting to the `levels` attribute directly (should not be strict depending on keywords as you proposed)

yeh, best to finish this issue off (and create a new issue, will be much more focused).

I DO like `set_levels`

Re API design: In most case I have the final list of levels (e.g. a lickert scale or "1 .. 5"), so I want a method which I can give my final levels and be done. 

``` python
scale = ["very good", "good", "neutral", "bad", "very bad"]
input =  ["very good", "good", "bad", "very good", "good", "good"] 
s = Series(input)
cat = s.astype("category")
# current idea / current 'reorder_levels'
cat = cat.change_levels(scale)
# without a "does all actions in one go"
removals = set(cat.levels) - set(scale)
cat = cat.remove_levels(removals)
additions = set(scale) - set(cat.levels)
cat = cat.add_levels(additions)
cat = cat.reorder_levels(scale)
```

I'm not a big fan of `set_...` as in this case I perceive add/ remove/ reorder as not a setting operation.

rebased o top of master
- removed the failing test on Py3 -> not sure what should happen in this case:

``` python
# Make sure we also convert special types of strings
arr = np.array(["a","b","c","a"], dtype="S1")
cat = Categorical(arr, ordered=True)
res = cat.reorder_levels(["c","b","a"])
self.assert_numpy_array_equal(res._codes , np.array([2,1,0,2])) # this is all NaN, as "a" != arr[0] on Py3
self.assert_numpy_array_equal(res.levels , np.array(["c","b","a"]))
self.assert_numpy_array_equal(res.get_values() , np.array(["a","b","c","a"]))
```
- changed s.cat.codes to a directly implemented property
- changed str(cat) to only display 10 values and add spaces after comma
- do the rename of 'levels' to 'categories'

Not yet done: docs, the points from above (rename/reorder/add/remove + general purpose method)...

@JanSchulz I think you have this PR and #8078 to cover the open issues, yes? lmk wher you are and when you want review

And #8138

I will also put in the above `rename_/reorder_/add_/remove_levels` and a `set_levels` and then go the doc cleanup/changes... don't want to go through docs twice...

@jreback @jorisvandenbossche I did the last todos, so IMO this is ready, but please review.

I kept the individual commits as distinct as possible to make reviewing easier and would really appreciate if this could go in without a squash... :-)

@jreback the last commit was a failing unittest on my windows system...

@JanSchulz pretty good!

pls just address (relatively minor) doc changes, and then can get this in. lmk as soon as you are ready. don't 'add' anything else. let's just get in and close the open issues. can do (small followups) if necessary.

@jreback ok, will do. Thanks for the review!

btw, in the very first doc section (and v0.15.0). I think if you could put a 'real' example of why someone would use a categorical, e.g. maybe sorting by the categorical in a particular order to show data a certain way - sort of the motivational example (no need to really explain it, just sort of show it so people get the ahah of _why_ one would use a categorical)