BUG: list-like to_replace on Categorical.replace is ignored or crash #31734

JustinZhengBC · 2020-02-06T04:41:51Z

closes Series.replace fails on categoricals with list #31720
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Covers the case where to_replace is a list-like and value is a string. Other cases, like "to_replace is dict and value is None", or "to_replace and value are both lists" are handled earlier in generic.py

MarcoGorelli · 2020-02-06T14:53:08Z

pandas/tests/arrays/categorical/test_replace.py

@@ -0,0 +1,44 @@
+import pandas as pd
+import pandas.testing as tm


Thanks @JustinZhengBC

This should be

import pandas._testing as tm

dsaxton · 2020-02-06T21:55:39Z

How does this behave when to_replace is already a dict? On master the replacement seems to work but the data type changes:

In [5]: s                                                                                                                                                             
Out[5]: 
0    0
1    1
2    2
dtype: category
Categories (3, int64): [0, 1, 2]

In [6]: s.replace({0: 1})                                                                                                                                             
Out[6]: 
0    1
1    1
2    2
dtype: int64

JustinZhengBC · 2020-02-07T04:41:28Z

@dsaxton this is because in NDFrame (generics.py) it chooses to not dispatch the dict case to the self._data.replace, but self._data.replace_list which takes a more complex path to iterate over its blocks and would need more drastic refactoring (it pre-computes masks to avoid repeated replacement if the same value appears more than once in to_replace or value). I think that is outside the scope of the current ticket

MarcoGorelli · 2020-02-08T21:48:21Z

Thanks @JustinZhengBC !

Overall looks good

MarcoGorelli · 2020-02-08T21:49:21Z

pandas/tests/arrays/categorical/test_replace.py

+        (1, 2, [2, 2, 3], True),
+        (1, 4, [4, 2, 3], True),
+        (4, 1, [1, 2, 3], True),
+        (5, 6, [1, 2, 3], True),


Does the case when to_replace and value are different dtypes need testing? e.g. with to_replace=1, value="4"

EDIT

actually, maybe that's outside the scope of the issue this PR is addressing

It's doable, with limited dtype-checking as a full check would involve the category index which causes a crash on comparison on mixed dtypes

MarcoGorelli · 2020-02-08T21:50:19Z

pandas/tests/arrays/categorical/test_replace.py

+        ([1, 4], [5, 2], [5, 2, 3], False),
+    ],
+)
+def test_replace(to_replace, value, expected, check_types):


Add the issue number here

MarcoGorelli · 2020-02-08T21:52:44Z

pandas/_testing.py

    check_categorical : bool, default True
        Whether to compare internal Categorical exactly.
+    check_category_order : bool, default True
+        Whether to compare category order internal Categoricals


Is there a missing "of" here? Should it be

Whether to compare category order of internal Categoricals

?

MarcoGorelli · 2020-02-08T21:59:52Z

pandas/core/arrays/categorical.py

+    def replace_list(self, to_replace, value, inplace: bool = False):
+        return self.replace(to_replace, value, inplace)
+


Where is this used?

sorry, this and the change below it were experiments in addressing the dict case and list-list case for categoricals, which turned out to be more complex than expected and are not intended to be in this pr

MarcoGorelli · 2020-02-08T22:04:08Z

pandas/core/arrays/categorical.py

+        if is_list_like(to_replace):
+            if is_list_like(value):
+                if len(to_replace) != len(value):
+                    raise ValueError("to_replace and value must be same length!")


Could this ValueError be a bit more precise? As in, include something about the lengths of to_replace and value which were received

sorry, this and the change above it were experiments in addressing the dict case and list-list case for categoricals, which turned out to be more complex than expected and are not intended to be in this pr

pep8speaks · 2020-02-08T22:24:05Z

Hello @JustinZhengBC! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-17 00:22:52 UTC

MarcoGorelli · 2020-02-09T09:03:52Z

pandas/tests/arrays/categorical/test_replace.py

+        ([1, 2, "3"], "5", ["5", "5", 3], True, False),
+    ],
+)
+# GH 31720


I think the issue number usually goes inside the test:

def test_me(*args): # GH 1234

Probably not worth changing on its own, but if you get any other comments requesting changes, perhaps this is worth doing

MarcoGorelli · 2020-02-09T09:06:19Z

doc/source/whatsnew/v1.1.0.rst

 ^^^^^^^^^^^

-
+- Bug in :class:`Categorical` that would ignore or crash when calling :meth:`Series.replace` with a list-like ``to_replace`` (:issue:`31720`)


The issue is tagged with v1.0.2 milestone, I think this should go in that whatsnew file

jreback

looks pretty good. move the note to 1.0.2

pandas/core/arrays/categorical.py

pandas/_testing.py

JustinZhengBC · 2020-02-09T20:43:06Z

@jreback I've moved the whatsnew and addressed the other feedback

pandas/core/arrays/categorical.py

jreback · 2020-02-12T12:43:27Z

also pls merge master

pandas/core/arrays/categorical.py

jreback

very minor comments, ping on green.

pandas/core/arrays/categorical.py

pandas/_testing.py

JustinZhengBC · 2020-02-17T01:07:50Z

@jreback green

jreback · 2020-02-17T17:00:00Z

thanks @JustinZhengBC very nice!

…l.replace is ignored or crash

…is ignored or crash (#32062) Co-authored-by: Justin Zheng <justinzhengbc@gmail.com>

…andas-dev#31734)

MarcoGorelli reviewed Feb 6, 2020

View reviewed changes

JustinZhengBC requested a review from MarcoGorelli February 8, 2020 20:33

MarcoGorelli reviewed Feb 8, 2020

View reviewed changes

JustinZhengBC requested a review from MarcoGorelli February 9, 2020 00:46

MarcoGorelli suggested changes Feb 9, 2020

View reviewed changes

jreback requested changes Feb 9, 2020

View reviewed changes

pandas/core/arrays/categorical.py Show resolved Hide resolved

pandas/_testing.py Show resolved Hide resolved

jreback added this to the 1.0.2 milestone Feb 9, 2020

jreback added Categorical Categorical Data Type Regression Functionality that used to work in a prior pandas version labels Feb 9, 2020

JustinZhengBC force-pushed the BUG-31720 branch from 10fd29e to fd0d736 Compare February 9, 2020 20:35

jreback requested changes Feb 12, 2020

View reviewed changes

pandas/core/arrays/categorical.py Show resolved Hide resolved

BUG: list-like to_replace on Categorical.replace is ignored or crash

7d3c1a3

JustinZhengBC force-pushed the BUG-31720 branch from fd0d736 to 7d3c1a3 Compare February 14, 2020 03:25

revert isna change

91d3ef6

jreback requested changes Feb 14, 2020

View reviewed changes

pandas/core/arrays/categorical.py Show resolved Hide resolved

JustinZhengBC added 2 commits February 15, 2020 15:56

move isna

ea41738

more isna stuff

e377776

jreback requested changes Feb 16, 2020

View reviewed changes

pandas/core/arrays/categorical.py Show resolved Hide resolved

pandas/_testing.py Show resolved Hide resolved

changes

8a2e12d

jreback approved these changes Feb 17, 2020

View reviewed changes

jreback merged commit 8444453 into pandas-dev:master Feb 17, 2020

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Feb 17, 2020

Backport PR pandas-dev#31734: BUG: list-like to_replace on Categorica…

ea047aa

…l.replace is ignored or crash

meeseeksmachine mentioned this pull request Feb 17, 2020

Backport PR #31734 on branch 1.0.x (BUG: list-like to_replace on Categorical.replace is ignored or crash) #32062

Merged

jreback added Still Needs Manual Backport and removed Still Needs Manual Backport labels Feb 17, 2020

jorisvandenbossche pushed a commit that referenced this pull request Feb 19, 2020

Backport PR #31734: BUG: list-like to_replace on Categorical.replace …

e3044d5

…is ignored or crash (#32062) Co-authored-by: Justin Zheng <justinzhengbc@gmail.com>

roberthdevries pushed a commit to roberthdevries/pandas that referenced this pull request Mar 2, 2020

BUG: list-like to_replace on Categorical.replace is ignored or crash (p…

b38c227

…andas-dev#31734)

TomAugspurger mentioned this pull request Mar 6, 2020

[Regression] DataFrame.replace throws TypeError when to_replace is list and DF has categorical #32497

Closed

jtilly mentioned this pull request Apr 4, 2020

REGR: Replacing a category with itself replaces it with np.nan #33288

Closed

3 tasks

		@@ -0,0 +1,44 @@
		import pandas as pd
		import pandas.testing as tm

		def replace_list(self, to_replace, value, inplace: bool = False):
		return self.replace(to_replace, value, inplace)

Uh oh!

BUG: list-like to_replace on Categorical.replace is ignored or crash #31734

BUG: list-like to_replace on Categorical.replace is ignored or crash #31734

Uh oh!

Conversation

JustinZhengBC commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsaxton commented Feb 6, 2020

Uh oh!

JustinZhengBC commented Feb 7, 2020

Uh oh!

MarcoGorelli commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoGorelli Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

EDIT

Uh oh!

JustinZhengBC Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pep8speaks commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-02-17 00:22:52 UTC

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JustinZhengBC commented Feb 9, 2020

Uh oh!

Uh oh!

jreback commented Feb 12, 2020

Uh oh!

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JustinZhengBC commented Feb 17, 2020

Uh oh!

jreback commented Feb 17, 2020

Uh oh!

JustinZhengBC commented Feb 6, 2020 •

edited

Loading

MarcoGorelli commented Feb 8, 2020 •

edited

Loading

MarcoGorelli Feb 8, 2020 •

edited

Loading

JustinZhengBC Feb 8, 2020 •

edited

Loading

MarcoGorelli Feb 8, 2020 •

edited

Loading

MarcoGorelli Feb 8, 2020 •

edited

Loading

pep8speaks commented Feb 8, 2020 •

edited

Loading

MarcoGorelli Feb 9, 2020 •

edited

Loading