DEPR: inplace dtype-changing methods on Categorical

[jbrockmendel]

• Categorical.remove_unused_categories Deprecate inplace in Categorical.remove_unused_categories #37918
• Cateogrical.remove_categories Deprecate inplace in Categorical.remove_categories #37981
• Categorical.add_categories Deprecate inplace in Categorical.add_categories. #41118
• Categorical.reorder_categories Deprecate inplace in Categorical.reorder_categories. #41133
• Categorical.rename_categories Deprecate inplace in Categorical.rename_categories #41186
• Categorical.set_categories Deprecate inplace in Categorical.set_categories. #41307

The ability to alter an existing array's dtype is a footgun. We can return a new object without having to make a copy.

[jorisvandenbossche]

Also set_categories.

All those methods have a inplace=False keyword, so by default they don't actually change the existing array's dtype. And with inplace=True this can be expected? (it's a bit similar issue as with inplace deprecation discussion, but also here a copy=True/False keyword might be more appropriate ...)

[AlexKirko]

Hard to think of a use case where changing a Categorical dtype in place would be intended, but easy to think of use cases where it would lead to unexpected errors. Probably best to deprecate this and raise.

[OlehKSS]

Hi, I've created a pull request to solve this issue for Categorical.remove_unused_categories. Could you check it?
For the other methods, should I create separate pull requests for each one of them so as to keep them small?

[OlehKSS]

take

[jorisvandenbossche]

I am not sure we should deprecate the inplace ability for all of those. For example, for set_categories, or rename_categories, this operation can actually happen inplace, and can thus be beneficial.

[TomAugspurger]

Agreed. It can be expensive to copy all the data (codes) when not necessary.

[jbrockmendel]

Agreed. It can be expensive to copy all the data (codes) when not necessary.

i was thinking we'd get a view on the codes, just a new Categorical object. This should be cheap.

[OlehKSS]

I've added another pull request to deprecate inplace in remove_categories.
As far as I understand, a deep copy of the underlying ndarray was created for inplace=False in set_categories and rename_categories.
Should I just keep the current behavior of these methods?

[jreback]

@jorisvandenbossche @TomAugspurger the inplace ops here seem like a footgun; sure they may have a small cost (but could be mitgated as @jbrockmendel indicates), but we don't allow anything like this on any other accessor operation. +1 on deprecation.

[OlehKSS]

@jbrockmendel
Should the inplace parameter be deprecated in the Categorical.replace method as well? This method uses Cateogrical.remove_categories internally

[jbrockmendel]

Without looking at the implementation: Categorical.replace should be able to alter the values (i.e. self._codes) inplace, but not able to alter the dtype (i.e. self._categories)

[ivirshup]

i was thinking we'd get a view on the codes, just a new Categorical object. This should be cheap.

It would be nice to be able to control this behaviour, maybe with copy. Right now, I'm not sure it's intuitive that inplace=False would return a view on the original data. Also methods like, remove_unused_categories modify the codes, so presumably this couldn't return a view unless the operation was mutating.

As a side note, I like the ability to modify these inplace – though I see that mutating the type (if that's what dtypes are) is strange.

[jbrockmendel]

all the checkboxes are checked; closing

[jbrockmendel]

Re-opening, never addressed the "option to return a view" part of the issue.

[jbrockmendel]

I'm having second thoughts about this. We currently allow setting categories using cat.categories = some_list, for which i don't see an obvious non-copy-but-not-inplace alternative.

The category-setting is a pretty convenient pattern, not sure how to trade this off against modifying-dtype-inplace being an antipattern.