Rename categories with Series #17982

TomAugspurger · 2017-10-25T16:49:26Z

head:

[ 50.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat         6.39ms
[100.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat_ordered 4.81ms

master:

[ 50.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat         137ms
[100.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat_ordered 5.14ms

HEAD: ``` [ 50.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat 6.63ms [ 50.00%] ····· [100.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat_ordered 4.85ms ``` Closes pandas-dev#17981

codecov · 2017-10-25T18:10:45Z

Codecov Report

Merging #17982 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17982      +/-   ##
==========================================
- Coverage   91.23%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       50113    50117       +4     
==========================================
- Hits        45723    45718       -5     
- Misses       4390     4399       +9

Flag	Coverage Δ
#multiple	`89.03% <100%> (ø)`	⬆️
#single	`40.3% <0%> (-0.08%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/categorical.py	`95.75% <100%> (+0.02%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <0%> (-0.1%)`	⬇️
pandas/core/ops.py	`91.77% <0%> (ø)`	⬆️
pandas/core/reshape/concat.py	`97.58% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36c309e...0e086ca. Read the comment docs.

jschendel · 2017-10-25T18:31:56Z

pandas/core/categorical.py

@@ -879,8 +874,22 @@ def rename_categories(self, new_categories, inplace=False):

        Parameters
        ----------
-        new_categories : Index-like or dict-like (>=0.21.0)
-           The renamed categories.
+        new_categories : array-like or dict-like


A bit of a nitpick: new_categories is listed as "array-like" here, but in the description below the term "list-like" is used instead, and then it's switched back to "array-like" further down in the examples section. Might be nice to make the terminology consistent, but certainly not something that's super critical.

Thanks, fixed.

jreback · 2017-10-25T22:30:27Z

lgtm. merge away.

jreback · 2017-10-25T22:31:08Z

you migh want to add the issue number to the original issue number in the whatsnew (#17336)

jorisvandenbossche · 2017-10-25T22:38:00Z

pandas/core/categorical.py

+        Categories (2, object): [A, b]
+
+        Series are considered list-like here, so the *values* are used
+        instead of the *index*


Do we actually want this behaviour?
Eg for Series.rename, a Series is seen as a dict-like ...

Not sure. It’ll be a backwards incompatible change if we don’t treat Series as arrays so I think we should at least do this for now, maybe with a warning.

I agree, I think this is fine.

@jreback which part to you agree with? Warning that it'll change to dict-like in the future?

no I agree the current behavior is correct. we handle list-like the same. no warning is needed as this is expected.

actually, looking at this again.

a Series should be just like a dict. This is a perf issue yes?
do this. (once Index.map works we could simplify a bit)

In [12]: cat = pd.Categorical(['a', 'b', 'c', 'd']) ...: res = cat.rename_categories(pd.Series({'a': 4, 'b': 3, 'c': 2, 'd': 1})) ...: ...: In [13]: cat Out[13]: [a, b, c, d] Categories (4, object): [a, b, c, d] In [14]: res Out[14]: [4, 3, 2, 1] Categories (4, int64): [4, 3, 2, 1] In [17]: pd.Series(cat.categories).map({'a': 4, 'b': 3, 'c': 2, 'd': 1}).values Out[17]: array([4, 3, 2, 1]) In [19]: pd.Series(cat.categories).map({'a': 4, 'b': 3, 'c': 2, 'd': 1}).values Out[19]: array([4, 3, 2, 1])

please revert the warning

Can you explain?
If we go for "Series -> dict-like" behaviour, this is a breaking change, and we need to use a warning for that.

jreback

see above

TomAugspurger · 2017-10-26T10:38:24Z

It's not really a perf issue, that's just the reason we caught it. The main issue is the API change from

c = pd.Categorical(['a', 'b'])
c.rename(pd.Series([10, 20)

Before #17586, the values would be used, and so the categories would be [10, 20]. Afterwards, the series is considered dict-like, so the index -> value mapping is used.

I really don't think we should just break API here. It'll be very surprising and difficult to debug. But in most cases Series is considered dict-like, so I think a warning is the best option here.

jreback · 2017-10-26T10:43:47Z

why don’t you just make th change to fix this back to how it was; this PR was in 0.21 so it’s not externally visible

warnings are a terrible things and should rarely be used

TomAugspurger · 2017-10-26T10:48:27Z

Just pushed a WIP adding the warning so that we're talking about the same thing, will clean up a bit further.

TomAugspurger · 2017-10-26T10:49:48Z

why don’t you just make th change to fix this back to how it was;

Not sure what you mean here, revert the entire PR? That's a useful feature, it just (unexpectedly) changes the behavior for Series. We didn't have any tests covering that, so it wasn't caught at the time.

jreback · 2017-10-26T10:50:06Z

pandas/core/categorical.py

+                   "For dict-like, use 'new_categories.to_dict()'\n"
+                   "For list-like, use 'new_categories.values'.")
+            warn(msg, FutureWarning, stacklevel=2)
+        if is_dict_like(new_categories) and not is_series:
            cat.categories = [new_categories.get(item, item)


just use map

jorisvandenbossche · 2017-10-26T11:10:53Z

pandas/core/categorical.py

-        new_categories : Index-like or dict-like (>=0.21.0)
-           The renamed categories.
+        new_categories : list-like or dict-like
+           The categories end up with


this seems like an incomplete sentence ?

jorisvandenbossche · 2017-10-26T11:14:09Z

pandas/core/categorical.py

+            categories.
+
+            If dict-like, categories not contained in the mapping are passed
+            through.


I find this a bit unstructured with the versionchanged and the further explanation splitted.

Suggestion (just copy paste, not edited sentences):

- list-like: If it is list-like, all items must be unique and the number of items in the new categories must match the existing number of categories. - .. versionadded:: 0.21.1 dict-like, in which case it specifies a mapping from old-categories to new. Categories not contained in the mapping are passed through. .. warning:: about series

(only not fully sure how the versionchanges works in a list)

jorisvandenbossche · 2017-10-26T11:15:09Z

pandas/core/categorical.py

+        Categories (2, object): [A, b]
+
+        Series are considered list-like here, so the *values* are used
+        instead of the *index*


Can you explain?
If we go for "Series -> dict-like" behaviour, this is a breaking change, and we need to use a warning for that.

jorisvandenbossche · 2017-10-26T11:15:59Z

pandas/core/categorical.py

+                   "treat Series like a dictionary.\n"
+                   "For dict-like, use 'new_categories.to_dict()'\n"
+                   "For list-like, use 'new_categories.values'.")
+            warn(msg, FutureWarning, stacklevel=2)


Maybe convert the series to array (list-like), so then the rest of the code does not need to take care of it being a series or not

If we go for "Series -> dict-like" behaviour, this is a breaking change, and we need to use a warning for that.

Sorry I think that was an example I added in the first commit of this PR, before we decided to treat Series as list-like.

why is this a breaking change at all? we simply did not support this before

The old behavior required a list-like, and Series are list like. It's not unreasonable for a user to expect

cat.rename(Series([0, 1]))

to work, since it did! But we have a new feature that changes the behavior.

ok I c, I think this was accidently supported before. ok so fine on the FutureWarning.

jorisvandenbossche · 2017-10-26T11:17:48Z

I find the current PR fine: warn for series to say that in future it will use dict-like behaviour, but for now keep list-like behaviour

TomAugspurger · 2017-10-26T17:54:02Z

@jreback are you OK with the FutureWarning for now?

jorisvandenbossche

Apart from some typos, looks good to me

jorisvandenbossche · 2017-10-26T21:25:51Z

doc/source/whatsnew/v0.21.0.txt

+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+:meth:`~Series.cat.rename_categories` now accepts a dict-like argument for
+``new_categories``. The previous categories are lookup up in the dictionary's


"lookup up" -> ""looked up"? (not sure what is the correct english conjugation)

jorisvandenbossche · 2017-10-26T21:28:08Z

pandas/core/categorical.py

+           * dict-like: specifies a mapping from
+             old categories to new. Categories not contained in the mapping
+             are passed through and extra categories in the mapping are
+             ignored. *New in verison 0.21.0*.


verison -> version

jreback · 2017-10-26T23:53:09Z

pandas/core/categorical.py

+                   "treat Series like a dictionary.\n"
+                   "For dict-like, use 'new_categories.to_dict()'\n"
+                   "For list-like, use 'new_categories.values'.")
+            warn(msg, FutureWarning, stacklevel=2)


ok I c, I think this was accidently supported before. ok so fine on the FutureWarning.

* PERF/API: Treat series as array-like for rename_categories HEAD: ``` [ 50.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat 6.63ms [ 50.00%] ····· [100.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat_ordered 4.85ms ``` Closes pandas-dev#17981 * Redo docstring * Use list-like * Warn * Fix doc indent * Doc cleanup * More doc cleanup * Fix API reference * Typos

TomAugspurger added 2 commits October 25, 2017 11:39

PERF/API: Treat series as array-like for rename_categories

b551b28

HEAD: ``` [ 50.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat 6.63ms [ 50.00%] ····· [100.00%] ··· Running categoricals.Categoricals3.time_rank_string_cat_ordered 4.85ms ``` Closes pandas-dev#17981

Redo docstring

19af62a

TomAugspurger added the Categorical Categorical Data Type label Oct 25, 2017

TomAugspurger added this to the 0.21.0 milestone Oct 25, 2017

TomAugspurger mentioned this pull request Oct 25, 2017

PEFR/BUG/API: rename_categories with a series #17981

Closed

TomAugspurger added the Performance Memory or execution speed performance label Oct 25, 2017

TomAugspurger mentioned this pull request Oct 25, 2017

PERF: Regressions since 0.20.3 #17861

Closed

jschendel reviewed Oct 25, 2017

View reviewed changes

Use list-like

f659e78

jorisvandenbossche reviewed Oct 25, 2017

View reviewed changes

jreback requested changes Oct 26, 2017

View reviewed changes

Warn

6460bc5

jreback requested changes Oct 26, 2017

View reviewed changes

Fix doc indent

506f9ba

jorisvandenbossche reviewed Oct 26, 2017

View reviewed changes

TomAugspurger added 2 commits October 26, 2017 06:29

Doc cleanup

4f6a1dd

More doc cleanup

cbcc723

Fix API reference

aec380d

TomAugspurger mentioned this pull request Oct 26, 2017

RLS: 0.21.0 #17285

Closed

64 tasks

jorisvandenbossche approved these changes Oct 26, 2017

View reviewed changes

Typos

0e086ca

jreback approved these changes Oct 26, 2017

View reviewed changes

jreback merged commit 7aeccd3 into pandas-dev:master Oct 26, 2017

jsexauer mentioned this pull request Oct 26, 2017

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

TomAugspurger deleted the rename-categories-bug branch October 27, 2017 11:43

jreback mentioned this pull request Jun 29, 2019

DEPR: deprecations log for removed issues #13777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename categories with Series #17982

Rename categories with Series #17982

TomAugspurger commented Oct 25, 2017 •

edited

Loading

codecov bot commented Oct 25, 2017 •

edited

Loading

jschendel Oct 25, 2017

TomAugspurger Oct 25, 2017

jreback commented Oct 25, 2017

jreback commented Oct 25, 2017

jorisvandenbossche Oct 25, 2017

TomAugspurger Oct 26, 2017

jreback Oct 26, 2017

TomAugspurger Oct 26, 2017

jreback Oct 26, 2017

jreback Oct 26, 2017

jreback Oct 26, 2017

jorisvandenbossche Oct 26, 2017

jreback left a comment

TomAugspurger commented Oct 26, 2017

jreback commented Oct 26, 2017

TomAugspurger commented Oct 26, 2017

TomAugspurger commented Oct 26, 2017

jreback Oct 26, 2017

jorisvandenbossche Oct 26, 2017

jorisvandenbossche Oct 26, 2017

jorisvandenbossche Oct 26, 2017

jorisvandenbossche Oct 26, 2017

TomAugspurger Oct 26, 2017

jreback Oct 26, 2017

TomAugspurger Oct 26, 2017

jreback Oct 26, 2017

jorisvandenbossche commented Oct 26, 2017

TomAugspurger commented Oct 26, 2017

jorisvandenbossche left a comment

jorisvandenbossche Oct 26, 2017

jorisvandenbossche Oct 26, 2017

jreback Oct 26, 2017

Rename categories with Series #17982

Rename categories with Series #17982

Conversation

TomAugspurger commented Oct 25, 2017 • edited Loading

codecov bot commented Oct 25, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 25, 2017

jreback commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

TomAugspurger commented Oct 26, 2017

jreback commented Oct 26, 2017

TomAugspurger commented Oct 26, 2017

TomAugspurger commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 26, 2017

TomAugspurger commented Oct 26, 2017

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Oct 25, 2017 •

edited

Loading

codecov bot commented Oct 25, 2017 •

edited

Loading