BUG-26988 implement replace for categorical blocks #27026

JustinZhengBC · 2019-06-24T23:22:15Z

closes Bug: Series[category].replace casts incorrectly #26988
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

The replace functions used in the code path that caused the bug report appeared to densify the series. This PR adds a replace function to CategoricalBlock that takes advantage of the categorical data type when possible

codecov · 2019-06-25T02:13:09Z

Codecov Report

Merging #27026 into master will decrease coverage by 0.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master   #27026      +/-   ##
==========================================
- Coverage   91.99%   91.98%   -0.02%     
==========================================
  Files         180      180              
  Lines       50774    50790      +16     
==========================================
+ Hits        46712    46719       +7     
- Misses       4062     4071       +9

Flag	Coverage Δ
#multiple	`90.62% <68.75%> (-0.01%)`	⬇️
#single	`41.79% <56.25%> (-0.12%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/internals/blocks.py	`94.17% <75%> (-0.22%)`	⬇️
pandas/io/gbq.py	`88.88% <0%> (-11.12%)`	⬇️
pandas/core/frame.py	`96.89% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.84% <0%> (-0.11%)`	⬇️
pandas/io/formats/format.py	`97.91% <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 83fe8d7...a2f2a7c. Read the comment docs.

codecov · 2019-06-25T02:13:16Z

Codecov Report

Merging #27026 into master will decrease coverage by 1.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #27026      +/-   ##
==========================================
- Coverage   93.07%   91.86%   -1.22%     
==========================================
  Files         192      180      -12     
  Lines       49562    50833    +1271     
==========================================
+ Hits        46130    46697     +567     
- Misses       3432     4136     +704

Flag	Coverage Δ
#multiple	`90.5% <93.33%> (-1.33%)`	⬇️
#single	`41.84% <46.66%> (-0.67%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/internals/blocks.py	`95.03% <100%> (-1.54%)`	⬇️
pandas/io/gbq.py	`88.88% <0%> (-11.12%)`	⬇️
pandas/core/computation/expressions.py	`93.27% <0%> (-5.81%)`	⬇️
pandas/compat/pickle_compat.py	`63.88% <0%> (-4.72%)`	⬇️
pandas/plotting/_matplotlib/timeseries.py	`66.32% <0%> (-3.79%)`	⬇️
pandas/core/computation/scope.py	`93.13% <0%> (-3.74%)`	⬇️
pandas/compat/__init__.py	`92% <0%> (-2.88%)`	⬇️
pandas/io/parquet.py	`65.59% <0%> (-2.55%)`	⬇️
pandas/io/formats/printing.py	`86.72% <0%> (-2.49%)`	⬇️
pandas/util/_decorators.py	`92.13% <0%> (-2.44%)`	⬇️
... and 182 more

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 04893a9...8667b0d. Read the comment docs.

jbrockmendel · 2019-06-25T15:27:25Z

doc/source/whatsnew/v0.25.0.rst


 - Bug in :func:`DataFrame.at` and :func:`Series.at` that would raise exception if the index was a :class:`CategoricalIndex` (:issue:`20629`)
 - Fixed bug in comparison of ordered :class:`Categorical` that contained missing values with a scalar which sometimes incorrectly resulted in ``True`` (:issue:`26504`)
+- Bug in :func:`DataFrame.replace` and :func:`Series.replace` that would give unusual results on categorical data (:issue:`26988`)


"unusual" --> "incorrect"

pandas/core/internals/blocks.py

pandas/tests/series/test_replace.py

jbrockmendel · 2019-06-25T15:31:58Z

pandas/core/internals/blocks.py


+    def replace(self, to_replace, value, inplace=False, filter=None,
+                regex=False, convert=True):
+        if filter is None or not self.mgr_locs.isin(filter).any():


this block looks unrelated to the linked issue. what case is it solving? needs a separate test?

The first case is used when the replace function applies to the entire block, so the replace can be carried out just by manipulating categories. Most calls to Series.replace should be covered with this case (including the new test in series/test_replace.py). If there's a filter, this might not be possible so we fix the categories then use the original implementation (second case)

I added a test in frame/test_replace.py to cover the second case, where a filter is used. It improves upon the previous behaviour, which would have casted the frame to object

JustinZhengBC · 2019-06-25T21:04:22Z

Failure unrelated #26842

pandas/core/internals/blocks.py

pandas/tests/frame/test_replace.py

jreback · 2019-06-27T03:01:35Z

pandas/tests/frame/test_replace.py

+        # GH 26988
+        df = DataFrame([[1, 1], [2, 2]], columns=['a', 'b'], dtype='category')
+        expected = DataFrame(final_data, columns=['a', 'b'], dtype='category')
+        expected['a'].cat.set_categories([1, 2, 3], inplace=True)


try not to use inplace in tests, its non-idiomatic.

…-26988

TomAugspurger

Fixed the merge conflict. LGTM.

jbrockmendel · 2019-07-08T17:06:21Z

Are there tests for the non-None filter branch?

JustinZhengBC · 2019-07-08T17:47:30Z

@jbrockmendel yes, the dataframe tests take that branch while the series tests do not

pandas/core/internals/blocks.py

jbrockmendel · 2019-07-10T00:03:24Z

I am -1 on this PR as is; this should be a method on a Categorical.

If it goes on Categorical, it should be generalized to EA.

jreback · 2019-07-10T00:22:08Z

I am -1 on this PR as is; this should be a method on a Categorical.

If it goes on Categorical, it should be generalized to EA.

probably. But I think its much better then jamming things in the blocks (now we already have replace on the blocks, this PR might be ok actually), but I don't fully remember.

TomAugspurger · 2019-07-15T17:02:06Z

I don't think we (or downstream EA authors) can reliably implement ExtensionArray.replace since it's such a complicated method.

We're also relying on Block.replace to do the heavy lifting. An ExtensionArray.replace wouldn't have that luxury.

jreback · 2019-09-08T15:57:41Z

can you merge master

jbrockmendel · 2019-09-09T15:06:22Z

will take a look this afternoon

jbrockmendel · 2019-09-09T15:38:42Z

doc/source/whatsnew/v1.0.0.rst

 ^^^^^^^^^^^

 - Added test to assert the :func:`fillna` raises the correct ValueError message when the value isn't a value from categories (:issue:`13628`)
+- Bug in :func:`DataFrame.replace` and :func:`Series.replace` that would give incorrect results on categorical data (:issue:`26988`)


"func" -> "meth"

jbrockmendel · 2019-09-09T15:43:12Z

pandas/core/arrays/categorical.py

+                cat.remove_categories(to_replace, inplace=True)
+            else:
+                index = categories.index(to_replace)
+                categories[index] = value


what if value is already in categories?

fixed with new case

jbrockmendel · 2019-09-09T15:48:57Z

Only major question is whether we need to add replace to the EA interface.

jbrockmendel · 2019-09-09T15:49:40Z

pandas/core/arrays/categorical.py

+        inplace = validate_bool_kwarg(inplace, "inplace")
+        cat = self if inplace else self.copy()
+        categories = cat.categories.tolist()
+        if to_replace in categories:


what happens if to_replace is not hashable? I think at the Categorical level we are OK raising, but at the Block level we would cast to object right?

Would casting to object help?

>>> import pandas as pd >>> s=pd.Series([1,2]).astype(object) >>> s.replace(1,[1,2,3]) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "pandas/core/series.py", line 4303, in replace method=method, File "pandas/core/generic.py", line 6819, in replace raise TypeError(msg) # pragma: no cover TypeError: Invalid "to_replace" type: 'int'

@jreback I've merged master and fixed all comments except this one for the reason above

jreback · 2019-10-06T23:36:56Z

this looked pretty good, can you merge master and address any remaining comments.

jreback · 2019-10-18T21:33:51Z

can you merge master

jbrockmendel · 2019-11-02T01:04:16Z

@JustinZhengBC can you merge master. itd be nice to get this in

JustinZhengBC · 2019-11-03T06:24:15Z

@jbrockmendel done

jreback

question about an error condition

jreback · 2019-11-04T14:54:41Z

pandas/core/arrays/categorical.py

        code_values = code_values[null_mask | (code_values >= 0)]
        return algorithms.isin(self.codes, code_values)

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


can you type these more specifically (can be a followon PR)

pandas/tests/frame/test_replace.py

JustinZhengBC · 2019-11-04T15:44:00Z

@jreback how should parameters like to_replace and value be typed when they can take on any type and might not even be the same type?

jreback · 2019-11-04T16:23:45Z

@jreback how should parameters like to_replace and value be typed when they can take on any type and might not even be the same type?

right, but they are something like Union[Scalar, AnyArrayLike] I think? (again this is likely tricky so should do as a followup)

TomAugspurger · 2019-11-12T20:39:17Z

Fixed the merge conflict.

I think we decided to leave typing as a followup, so this should be good to go?

jbrockmendel · 2019-11-12T21:55:21Z

pandas/core/internals/blocks.py

+                result.values.add_categories(value, inplace=True)
+            return super(CategoricalBlock, result).replace(
+                to_replace, value, inplace, filter, regex, convert
+            )


can any of the logic in this method generalize to ExtensionBlock? (presumably we'd need to add replace to the EA interface?)

this is a good point, but we can certainly do as a followup; can you open an issue for discussion.

jreback · 2019-11-13T01:51:37Z

@JustinZhengBC if you can address @jbrockmendel comments to add some comments this lgtm and we can merge. (alternatvely @jbrockmendel could do as a followup)

JustinZhengBC · 2019-11-13T02:45:35Z

@jreback @jbrockmendel I added the comment

alimcmaster1 · 2019-11-16T02:03:27Z

Seems like this is good to merge.
CC. @jreback @jbrockmendel

jreback · 2019-11-16T21:54:12Z

thanks for the patch @JustinZhengBC very nice!

JustinZhengBC added 2 commits June 24, 2019 16:20

implement replace for categorical blocks

770ad53

handle when value is none

a2f2a7c

jbrockmendel reviewed Jun 25, 2019

View reviewed changes

pandas/core/internals/blocks.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Jun 25, 2019

View reviewed changes

pandas/tests/series/test_replace.py Show resolved Hide resolved

jbrockmendel reviewed Jun 25, 2019

View reviewed changes

JustinZhengBC added 4 commits June 25, 2019 11:06

fix inplace arg

535ab5d

fix whatsnew

9482c2b

simplify filter

702e71d

add test

da778e1

gfyoung added Bug Categorical Categorical Data Type Dtype Conversions Unexpected or buggy dtype conversions labels Jun 26, 2019

jreback requested changes Jun 27, 2019

View reviewed changes

JustinZhengBC and others added 3 commits June 30, 2019 00:02

improve tests

bec92d0

fix other instance of inplace in test

593e1e0

Merge remote-tracking branch 'upstream/master' into JustinZhengBC-BUG…

5f23dc9

…-26988

TomAugspurger added this to the 0.25.0 milestone Jul 8, 2019

TomAugspurger approved these changes Jul 8, 2019

View reviewed changes

jreback removed this from the 0.25.0 milestone Jul 9, 2019

jreback requested changes Jul 9, 2019

View reviewed changes

pandas/core/internals/blocks.py Show resolved Hide resolved

jbrockmendel reviewed Sep 9, 2019

View reviewed changes

JustinZhengBC added 2 commits October 20, 2019 00:02

merge master

8667b0d

fix formatting

988ebc3

Merge branch 'master' into BUG-26988

d8a8d1f

jreback reviewed Nov 4, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into BUG-26988

0154bba

jbrockmendel reviewed Nov 12, 2019

View reviewed changes

jreback approved these changes Nov 13, 2019

View reviewed changes

add comment

c4d3f84

jreback merged commit fb08cee into pandas-dev:master Nov 16, 2019

Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019

BUG-26988 implement replace for categorical blocks (pandas-dev#27026)

3225cec

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

BUG-26988 implement replace for categorical blocks (pandas-dev#27026)

e5ce3d1

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

BUG-26988 implement replace for categorical blocks (pandas-dev#27026)

3f7d1ca

bnaul mentioned this pull request Feb 5, 2020

Series.replace fails on categoricals with list #31720

Closed

simonjayhawkins mentioned this pull request Apr 5, 2020

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

Closed

3 tasks

Uh oh!

BUG-26988 implement replace for categorical blocks #27026

BUG-26988 implement replace for categorical blocks #27026

Uh oh!

Conversation

JustinZhengBC commented Jun 24, 2019

Uh oh!

codecov bot commented Jun 25, 2019

Codecov Report

Uh oh!

codecov bot commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JustinZhengBC Jun 25, 2019 • 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!

JustinZhengBC commented Jun 25, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomAugspurger left a comment

Choose a reason for hiding this comment

Uh oh!

jbrockmendel commented Jul 8, 2019

Uh oh!

JustinZhengBC commented Jul 8, 2019

Uh oh!

Uh oh!

jbrockmendel commented Jul 10, 2019

Uh oh!

jreback commented Jul 10, 2019

Uh oh!

TomAugspurger commented Jul 15, 2019

Uh oh!

jreback commented Sep 8, 2019

Uh oh!

jbrockmendel commented Sep 9, 2019

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!

jbrockmendel commented Sep 9, 2019

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!

jreback commented Oct 6, 2019

Uh oh!

jreback commented Oct 18, 2019

Uh oh!

jbrockmendel commented Nov 2, 2019

Uh oh!

JustinZhengBC commented Nov 3, 2019

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jun 25, 2019 •

edited

Loading

JustinZhengBC Jun 25, 2019 •

edited

Loading