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

@@ -1104,6 +1105,8 @@ def assert_series_equal(
        Compare datetime-like which is comparable ignoring dtype.
    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

@@ -99,7 +99,7 @@ Bug fixes
 Categorical
 ^^^^^^^^^^^

-
+- 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 <[email protected]>

…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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

JustinZhengBC commented Feb 6, 2020 •

edited

Loading

MarcoGorelli Feb 6, 2020

dsaxton commented Feb 6, 2020

JustinZhengBC commented Feb 7, 2020

MarcoGorelli commented Feb 8, 2020 •

edited

Loading

MarcoGorelli Feb 8, 2020 •

edited

Loading

JustinZhengBC Feb 8, 2020 •

edited

Loading

MarcoGorelli Feb 8, 2020

JustinZhengBC Feb 8, 2020

MarcoGorelli Feb 8, 2020 •

edited

Loading

JustinZhengBC Feb 8, 2020

MarcoGorelli Feb 8, 2020

JustinZhengBC Feb 8, 2020

MarcoGorelli Feb 8, 2020 •

edited

Loading

JustinZhengBC Feb 8, 2020

pep8speaks commented Feb 8, 2020 •

edited

Loading

MarcoGorelli Feb 9, 2020

JustinZhengBC Feb 9, 2020

MarcoGorelli Feb 9, 2020 •

edited

Loading

JustinZhengBC Feb 9, 2020

jreback left a comment

JustinZhengBC commented Feb 9, 2020

jreback commented Feb 12, 2020

jreback left a comment

JustinZhengBC commented Feb 17, 2020

jreback commented Feb 17, 2020

		@@ -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)

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

Conversation

JustinZhengBC commented Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

dsaxton commented Feb 6, 2020

JustinZhengBC commented Feb 7, 2020

MarcoGorelli commented Feb 8, 2020 • edited Loading

MarcoGorelli Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

EDIT

JustinZhengBC Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli Feb 8, 2020 • edited Loading

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

MarcoGorelli Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Feb 8, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli Feb 9, 2020 • edited Loading

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

JustinZhengBC commented Feb 9, 2020

jreback commented Feb 12, 2020

jreback left a comment

Choose a reason for hiding this comment

JustinZhengBC commented Feb 17, 2020

jreback commented Feb 17, 2020

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