BUG: fixed .str.contains(..., na=False) for categorical series #22170

pulkitmaloo · 2018-08-02T14:02:13Z

closes .str.contains(..., na=False) consistency between categorical and object series #22158
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

TomAugspurger · 2018-08-14T14:58:38Z

Failure is at https://travis-ci.org/pandas-dev/pandas/jobs/411345500#L2287-L2289

Basically, you're fix only works if there are missing values.

This may fix it, but may break other things

diff --git a/pandas/core/strings.py b/pandas/core/strings.py
index 8097ed196..06fd1391e 100644
--- a/pandas/core/strings.py
+++ b/pandas/core/strings.py
@@ -1855,7 +1855,12 @@ class StringMethods(NoNewAttributesMixin):
         # before the transformation...
         if use_codes and self._is_categorical:
             result = take_1d(result, self._orig.cat.codes)
-            result[isna(result)] = na
+            missing = isna(result)
+
+            if missing.any():
+                result_type = np.result_type(result, na)
+                result = result.astype(result_type, copy=False)
+                result[isna(result)] = na
 
         if not hasattr(result, 'ndim') or not hasattr(result, 'dtype'):
             return result

pep8speaks · 2018-08-17T18:58:08Z

Hello @pulkitmaloo! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/strings.py !
There are no PEP8 issues in the file pandas/tests/test_strings.py !

Comment last updated on September 24, 2018 at 01:15 Hours UTC

TomAugspurger · 2018-08-18T19:16:58Z

Looks like a maybe unrelated CI failure. Can you merge master and push again?

TomAugspurger · 2018-08-18T19:21:15Z

And can you ensure (and test) that na is handled correctly for all the string functions? It looks like at least match isn't handled correctly.

…ategorical_bug

codecov · 2018-08-24T14:14:03Z

Codecov Report

❗ No coverage uploaded for pull request base (master@960a73f). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #22170   +/-   ##
=========================================
  Coverage          ?   92.19%           
=========================================
  Files             ?      169           
  Lines             ?    50950           
  Branches          ?        0           
=========================================
  Hits              ?    46975           
  Misses            ?     3975           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.62% <100%> (?)`
#single	`42.27% <0%> (?)`

Impacted Files	Coverage Δ
pandas/core/strings.py	`98.58% <100%> (ø)`

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 960a73f...44b36a4. Read the comment docs.

… into categorical_bug

jreback · 2018-09-18T13:27:24Z

ci/circle-27-compat.yaml

  - cython=0.28.2
  - jinja2=2.8
  - numexpr=2.4.4 # we test that we correctly don't use an unsupported numexpr
-  - numpy=1.9.2


can you revert this (and the rest of the .yaml)

pandas/core/strings.py

pandas/tests/test_strings.py

… into categorical_bug

pulkitmaloo · 2018-09-23T11:33:59Z

@jreback Thanks for the feedback, I have updated the PR as you requested. However, the changes in yaml files leads to circleci failure.

jreback · 2018-09-23T13:13:18Z

@pulkitmaloo you need to rebase on master

… into categorical_bug

jreback

looks good, can you add a whatsnew note in bug fixes

jreback · 2018-09-25T14:55:28Z

doc/source/whatsnew/v0.24.0.txt

 ^^^^^^^

-
+- Bug in :class:`StringMethods` where `str_contains()` was not filling missing values with given argument for na for a categorical Series (:issue:`22158`)


can you use double backticks on Series, can you use the actual reference here, e.g. :func:`Series.str.contains`

@jreback Thanks, I've updated it. Please let me know if there's anything else you'd like to change.

@jreback Can you please review and merge this PR.

doc/source/whatsnew/v0.24.0.txt

pandas/tests/test_strings.py

Co-Authored-By: pulkitmaloo <maloo.pulkit@gmail.com>

TomAugspurger · 2018-11-02T17:21:47Z

pandas/tests/test_strings.py

+        values = Series(["a", "b", "c", "a", np.nan], dtype="category")
+        result = values.str.contains('a', na=True).astype(object)
+        expected = Series([True, False, False, True, True], dtype=np.object_)
+        assert isinstance(result, Series)


this can be removed. The assert_series_equal checks that.

TomAugspurger · 2018-11-02T17:23:42Z

pandas/tests/test_strings.py

-        assert res.loc[2] == "foo"
+        # na for category
+        values = Series(["a", "b", "c", "a", np.nan], dtype="category")
+        result = values.str.contains('a', na=True).astype(object)


Why the .astype(object). What's the dtype when you don't do that? Categorical? If so, I'd rather we document that as part of the API and test it.

TomAugspurger · 2018-11-02T17:24:04Z

pandas/tests/test_strings.py

+
+        # na for objects
+        values = Series(["a", "b", "c", "a", np.nan])
+        result = values.str.contains('a', na=True).astype(object)


Same comment for the astype object.

TomAugspurger · 2018-11-09T16:13:39Z

@pulkitmaloo are you able to update?

jreback · 2018-11-18T22:19:57Z

@TomAugspurger should be fixed up.

jreback · 2018-11-20T01:14:01Z

thanks @pulkitmaloo

…s-dev#22170)

pulkitmaloo added 2 commits August 2, 2018 09:48

BUG: fixed .str.contains(..., na=False) for categorical series

077d136

na argument for _wrap_results

2ae44d1

datapythonista added Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Strings String extension data type and string data Categorical Categorical Data Type labels Aug 2, 2018

fixed str.contains for missing values

a1b3d7b

PEP8 Issue: added whitespace after ','

dbd990b

Merge branch 'master' of https://github.com/pulkitmaloo/pandas into c…

9d5d2c2

…ategorical_bug

pulkitmaloo added 8 commits August 24, 2018 14:32

na argument for wrap_results in match

90aef7b

BUG: fixed .str.contains(..., na=False) for categorical series

69f16af

na argument for _wrap_results

78cf8c7

fixed str.contains for missing values

93bb24a

PEP8 Issue: added whitespace after ','

6c2700f

na argument for wrap_results in match

6649129

Merge branch 'categorical_bug' of https://github.com/pulkitmaloo/pandas…

5c87e81

… into categorical_bug

Merge branch 'master' into categorical_bug

a09dcc5

jreback requested changes Sep 18, 2018

View reviewed changes

pandas/tests/test_strings.py Outdated Show resolved Hide resolved

pulkitmaloo added 5 commits September 22, 2018 15:27

Update circle-27-compat.yaml

3abdea5

Update travis-27-locale.yaml

d136599

Merge branch 'categorical_bug' of https://github.com/pulkitmaloo/pandas…

b942e16

… into categorical_bug

added tests for na arg for categorical and objects

82f9b9e

updated _wrap_results with arg fill_value and removed na

53e9253

pulkitmaloo added 8 commits September 23, 2018 20:50

PEP8 Issue: added whitespace after ','

f6cb04f

na argument for wrap_results in match

1f0256a

Update travis-27-locale.yaml

7025f34

added tests for na arg for categorical and objects

7542448

updated _wrap_results with arg fill_value and removed na

f1b4274

Merge branch 'categorical_bug' of https://github.com/pulkitmaloo/pandas…

386ab98

… into categorical_bug

Update travis-27-locale.yaml

7a09c44

fixed line too long

3408920

jreback added this to the 0.24.0 milestone Sep 25, 2018

jreback requested changes Sep 25, 2018

View reviewed changes

whatsnew note

3288d11

jreback requested changes Sep 25, 2018

View reviewed changes

Update v0.24.0.txt

6c87770

TomAugspurger reviewed Oct 18, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.txt Outdated Show resolved Hide resolved

jreback requested changes Oct 18, 2018

View reviewed changes

pandas/tests/test_strings.py Show resolved Hide resolved

Update doc/source/whatsnew/v0.24.0.txt

d242647

Co-Authored-By: pulkitmaloo <maloo.pulkit@gmail.com>

TomAugspurger reviewed Nov 2, 2018

View reviewed changes

TomAugspurger mentioned this pull request Nov 9, 2018

Unexpected behavior with NaN values on str operators with categorical data #23602

Closed

jreback added 3 commits November 18, 2018 17:10

Merge branch 'master' into PR_TOOL_MERGE_PR_22170

def1b4e

whatsnew

fd99431

cleanup

44b36a4

jreback approved these changes Nov 18, 2018

View reviewed changes

jreback merged commit 2af56d4 into pandas-dev:master Nov 20, 2018

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: fixed .str.contains(..., na=False) for categorical series (panda…

4072ed5

…s-dev#22170)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: fixed .str.contains(..., na=False) for categorical series (panda…

ebbca06

…s-dev#22170)

Uh oh!

BUG: fixed .str.contains(..., na=False) for categorical series #22170

BUG: fixed .str.contains(..., na=False) for categorical series #22170

Uh oh!

Conversation

pulkitmaloo commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomAugspurger commented Aug 14, 2018

Uh oh!

pep8speaks commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on September 24, 2018 at 01:15 Hours UTC

Uh oh!

TomAugspurger commented Aug 18, 2018

Uh oh!

TomAugspurger commented Aug 18, 2018

Uh oh!

codecov bot commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jreback Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pulkitmaloo commented Sep 23, 2018

Uh oh!

jreback commented Sep 23, 2018

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

jreback Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

pulkitmaloo Sep 26, 2018 • 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!

pulkitmaloo Oct 18, 2018 • 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!

Uh oh!

Uh oh!

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

TomAugspurger commented Nov 9, 2018

Uh oh!

jreback commented Nov 18, 2018

Uh oh!

jreback commented Nov 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pulkitmaloo commented Aug 2, 2018 •

edited

Loading

pep8speaks commented Aug 17, 2018 •

edited

Loading

codecov bot commented Aug 24, 2018 •

edited

Loading

pulkitmaloo Sep 26, 2018 •

edited

Loading

pulkitmaloo Oct 18, 2018 •

edited

Loading