BUG: fix str.replace('.','') should replace every character #24935

charlesdong1991 · 2019-01-25T18:30:44Z

closes str.replace('.','') should replace every character? #24804
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2019-01-25T18:30:52Z

Hello @charlesdong1991! 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 2019-06-13 00:02:24 UTC

codecov · 2019-01-25T19:45:03Z

Codecov Report

Merging #24935 into master will decrease coverage by 49.48%.
The diff coverage is 28.57%.

@@             Coverage Diff             @@
##           master   #24935       +/-   ##
===========================================
- Coverage   92.38%   42.89%   -49.49%     
===========================================
  Files         166      166               
  Lines       52388    52392        +4     
===========================================
- Hits        48397    22472    -25925     
- Misses       3991    29920    +25929

Flag	Coverage Δ
#multiple	`?`
#single	`42.89% <28.57%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/strings.py	`32.81% <28.57%> (-65.78%)`	⬇️
pandas/io/formats/latex.py	`0% <0%> (-100%)`	⬇️
pandas/core/categorical.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas_constants.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/plotting.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/converter.py	`0% <0%> (-100%)`	⬇️
pandas/io/formats/html.py	`0% <0%> (-99.35%)`	⬇️
pandas/core/groupby/categorical.py	`0% <0%> (-95.46%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-91.17%)`	⬇️
pandas/io/sas/sas_xport.py	`0% <0%> (-90.15%)`	⬇️
... and 124 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 83eb242...8061fda. Read the comment docs.

codecov · 2019-01-25T19:45:08Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #24935   +/-   ##
=========================================
  Coverage          ?   41.12%           
=========================================
  Files             ?      179           
  Lines             ?    50709           
  Branches          ?        0           
=========================================
  Hits              ?    20854           
  Misses            ?    29855           
  Partials          ?        0

Flag	Coverage Δ
#single	`41.12% <20%> (?)`

Impacted Files	Coverage Δ
pandas/core/reshape/melt.py	`12.6% <0%> (ø)`
pandas/core/strings.py	`37.76% <25%> (ø)`

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 d66da60...6267aae. Read the comment docs.

jreback

there are no tests for the deprecation

pandas/core/reshape/melt.py

jreback · 2019-01-26T15:24:24Z

doc/source/whatsnew/v0.25.0.rst

@@ -114,10 +114,8 @@ Conversion

 Strings
 ^^^^^^^
+- Bug in :func:`Series.str.replace` not applying regex in patterns of length 1 (:issue:`24804`)


you can leave this here, but need a note on the Deprecation warning change.

pandas/core/strings.py

jreback · 2019-01-26T15:26:31Z

pandas/core/strings.py

@@ -456,9 +456,10 @@ def str_replace(arr, pat, repl, n=-1, case=None, flags=0, regex=True):
    flags : int, default 0 (no flags)
        - re module flags, e.g. re.IGNORECASE
        - Cannot be set if `pat` is a compiled regex
-    regex : boolean, default True
+    regex : boolean, default None
        - If True, assumes the passed-in pattern is a regular expression.
        - If False, treats the pattern as a literal string


this is a disconcerting change. You are essentially having a different default on what is being passed here. I would be ok with forcing the passing of regex always, possibly in preparation of changing the default to regex=False

Yes, I understand that @jreback it's a quite disruptive change. This change is following the proposal from @TomAugspurger in #24809 (comment) . The purpose is to make this argument more explicit.

I would be ok with forcing the passing of regex always, possibly in preparation of changing the default to regex=False

@jreback can you clarify what you mean by "forcing"? If we raise when regex is not specified, that would be an API breaking change.

My proposal is the preserve the previous behavior, but warn when a length-1 regex is detected. Then we can get to the documented behavior of regex=True.

TomAugspurger · 2019-01-28T15:49:57Z

pandas/core/strings.py

        - If True, assumes the passed-in pattern is a regular expression.
        - If False, treats the pattern as a literal string
+        - If pat is single special character, default regex is False.


This should mention something about a warning being issued when regex is not specified. Something like

- If `pat` is a single character and `regex` is not specified, `pat` is interpreted as a string literal. If `pat` is also a regular expression symbol, a warning is issued that in the future `pat` will be interpreted as a regex, rather than a literal.

TomAugspurger · 2019-01-28T15:54:00Z

pandas/core/strings.py

@@ -577,6 +578,12 @@ def str_replace(arr, pat, repl, n=-1, case=None, flags=0, regex=True):
        if callable(repl):
            raise ValueError("Cannot use a callable replacement when "
                             "regex=False")
+        # if regex is default None, and a single special character is given
+        # in pat, still take it as a literal, and raise the Future warning
+        if regex is None and len(pat) == 1 and re.findall(r"\W", pat):


I don't understand the '\W' here. I think that's not quite right for non-word characters that are not regex symbols. Something like @

Does python have a list of regex symbols?

Is the heuristic re.escape(pat) == pat good enough?

In [46]: re.escape('@') Out[46]: '@' In [47]: re.escape('^') Out[47]: '\\^'

@TomAugspurger oh, indeed it's wrong! nice catch!! thanks for the correction!!

Can you add a test to ensure that something like Series.str.replace('@', 'at') doesn't produce a warning?

sure! @TomAugspurger

ehh, @TomAugspurger escape somehow changed... wired
i did get same output the first time i tested on my jupyter notebook:
re.escape("@") -> "@"
but now, i cannot replicate this and get re.escape("@") -> "\\@" ...
so i will go for this option:
pat in list("[\^$.|?*+()]")

TomAugspurger · 2019-01-28T16:16:40Z

doc/source/whatsnew/v0.25.0.rst

-
-
-
+- :func:`Series.str.replace`, the default regex for single character might be deprecated.


This will need to be updated.

charlesdong1991 · 2019-02-04T17:42:27Z

any follow-up code change requests and reviews? @jreback @TomAugspurger

jreback

I would rather simply go whole hog and just change regex=None as a warning for all, meaning you must change to an explicit True/False. It makes it more clear. But this would then be a rather large change.

jreback · 2019-02-16T18:00:24Z

doc/source/whatsnew/v0.25.0.rst

@@ -42,6 +42,7 @@ Other API Changes
 Deprecations
 ~~~~~~~~~~~~

+- :func:`Series.str.replace`, when pat is single special regex character and regex is not defined, regex is by default False for now, but this might be deprecated in the future.


needs an issue reference; use double backticks on False, use single backticks on keywordds.

thanks changed!

charlesdong1991 · 2019-02-16T18:41:55Z

yes, indeed will be a kind of api change. shall I open another issue to make this change if other maintainers agree with this? Or do you want to do this change in this PR? @jreback

WillAyd · 2019-02-28T00:40:03Z

So as an alternate what if we simply detected the case where the pattern provided is a single / special regex character with regex=True and raise a FutureWarning when that happens, noting that we will change the behavior to evaluate that as a regular expression regardless in the future

charlesdong1991 · 2019-02-28T14:01:06Z

@WillAyd based on your description, i think it's kind of what this PR is doing, right?

WillAyd · 2019-02-28T16:46:15Z

I'm saying don't even update the signature of the function, simply do something like:

if regex=True and pater in re_char_blacklist:
    raise FutureWarning("Single character patterns will be evaluated as regexes in the future")

Or something to the effect. Would minimize a lot of the volatility going on here

TomAugspurger · 2019-02-28T17:05:04Z

@WillAyd the raise in your pseudocode should probably be a warnings.warn?

I (think?) then there is no way to get the desired future behavior without warning. IIUC, your prosposal would warn on

pd.Series(['aa']).str.replace('.', 'b', regex=True)

I think we'll need a period of

regex=None (default): warning and output aa
regex=True: no warning, output bb
regex=False: no warning, output aa

WillAyd · 2019-02-28T17:06:30Z

Yea sorry meant warn instead of raise. Why do we need regex=None at all? I was hoping to avoid that API change

TomAugspurger · 2019-02-28T17:08:02Z

Does the rest of
#24935 (comment) answer your question? The short version is (IIUC), your proposal wouldn't allow for option 2 (regex=True interprets . as a regex, and doest not warn).

WillAyd · 2019-02-28T17:34:33Z

Hmm unless I'm mistaken my proposal would only affect option two. So something like:

if regex=True and pater in {'.', '^', '$', '*', '+', '?', '}'}:
   warnings.warn("You've passed a single special character in with regex=True which is evaluated as a single character. This will change in the future to match regex behavior")

Warns the user about this behavior in the future but doesn't impact their current code, allowing for a smoother deprecation in the future.

To your point though the result of option 2 would still stay 'aa' as is today just with the warning

TomAugspurger · 2019-02-28T17:36:52Z

Right, so I think we understand each other. My argument is that we shouldn't add a warning without a concrete action a user can take to avoid it. Else it'll just annoy them :)

…

On Thu, Feb 28, 2019 at 11:34 AM William Ayd ***@***.***> wrote: Hmm unless I'm mistaken my proposal would only affect option two. So something like: if regex=True and pater in {'.', '^', '$', '*', '+', '?', '}'}: warnings.warn("You've passed a single special character in with regex=True which is evaluated as a single character. This will change in the future to match regex behavior") Warns the user about this behavior in the future but doesn't impact their current code, allowing for a smoother deprecation in the future. To your point though the result of option 2 would still stay 'aa' as is today just with the warning — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24935 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIhcVxvePpsXFK79RPv9m5d1NITnsks5vSBMugaJpZM4aTbKn> .

WillAyd · 2019-02-28T17:48:13Z

Ah I gotcha. So in my mind if a user is passing a single special character and regex=True (whether explicitly or via the default argument) then it is a current "misuse" of that parameter.

If the warning said something like "explicitly pass regex=False to maintain current behavior" would that be better?

TomAugspurger · 2019-02-28T20:14:00Z

No worries. I'd recommend waiting, I don't think a decision has been made yet.

…

On Thu, Feb 28, 2019 at 2:07 PM Kaiqi Dong ***@***.***> wrote: Many thanks for your feedback and discussions @TomAugspurger <https://github.com/TomAugspurger> @WillAyd <https://github.com/WillAyd> My English is not very good, and i might get distracted by your discussions, could you give me any hint on what kind of change I need to make (or give a general direction to make me a bit clearer)? as far as i understand from your discussions, seems warning messages should be changed a bit? @WillAyd <https://github.com/WillAyd> @TomAugspurger <https://github.com/TomAugspurger> many thanks and sorry for asking this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24935 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIuj6ifkRxKYOEfYoCbJ-HchcPieaks5vSDbvgaJpZM4aTbKn> .

jreback · 2019-03-03T03:05:30Z

doc/source/whatsnew/v0.25.0.rst

@@ -43,6 +43,7 @@ Other API Changes
 Deprecations
 ~~~~~~~~~~~~

+- :func:`Series.str.replace`, when pat is single special regex character and regex is not defined, regex is by default ``False`` for now, but this might be deprecated in the future. (:issue:`24804`)


use double back-ticks around pat.

..is single special regex character'.. (such as .|\ etc).

jreback · 2019-03-03T03:06:33Z

pandas/core/strings.py

@@ -456,9 +456,13 @@ def str_replace(arr, pat, repl, n=-1, case=None, flags=0, regex=True):
    flags : int, default 0 (no flags)
        - re module flags, e.g. re.IGNORECASE
        - Cannot be set if `pat` is a compiled regex
-    regex : bool, default True
+    regex : boolean, default None


default is still True

so, you mean we now dont raise a warning anymore when char is a special single character while user doesn't explicitly specify regex?
if set default to True, and i use @TomAugspurger example:

pd.Series(['aa']).str.replace('.', 'b')

we will get:

default or explicitly regex=True, output is bb, no warning

regex=False, output is aa, no warning.
is it correct? @WillAyd @TomAugspurger @jreback

TomAugspurger · 2019-03-03T13:08:21Z

@WillAyd can you summarize your aversion to the (typical) use of None as a backwards compatible sentinel value for when we should warn?

WillAyd · 2019-03-03T22:10:38Z

Not terribly against it if that’s our process

…

Sent from my iPhone

On Mar 3, 2019, at 5:08 AM, Tom Augspurger ***@***.***> wrote: @WillAyd can you summarize your aversion to the (typical) use of None as a backwards compatible sentinel value for when we should warn? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jreback · 2019-03-20T02:01:26Z

can you merge master

charlesdong1991 · 2019-03-24T15:29:18Z

Hi, @jreback I just merged master and fix conflict!

jreback · 2019-03-24T21:22:14Z

something still failing

charlesdong1991 · 2019-03-31T11:29:15Z

@jreback i just merged the master and fix test error

WillAyd · 2019-04-10T04:54:48Z

This one has been around for a while. Just so we are all on the same page - is the intention here definitely to force users down the road to explicitly provide regex=True or regex=False? I know that was mentioned in comments above just not sure we agreed to it.

An easy alternative would just be to raise a DeprecationWarning when a single special character gets passed and actually change behavior at a later date, which I think is what @TomAugspurger suggests in #24804 (comment)

TomAugspurger · 2019-04-10T11:46:00Z

I don’t see a reason to make the regex flag required.

…

On Apr 9, 2019, at 23:54, William Ayd ***@***.***> wrote: This one has been around for a while. Just so we are all on the same page - is the intention here definitely to force users down the road to explicitly provide regex=True or regex=False? I know that was mentioned in comments above just not sure we agreed to it. An easy alternative would just be to raise a DeprecationWarning when a single special character gets passed and actually change behavior at a later date, which I think is what @TomAugspurger suggests in #24804 (comment) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jreback · 2019-04-10T11:58:56Z

ok, re-reading: #24809 (comment)

that looks like a good idea, not sure where that is in implementation.

charlesdong1991 · 2019-04-12T13:20:01Z

@jreback thanks for your comment! could you explain a little bit more? I think the change I made in this PR was following @TomAugspurger suggestion in 24809

jreback · 2019-06-08T20:20:52Z

@charlesdong1991 can you merge master and we'll see where this is

charlesdong1991 · 2019-06-12T09:16:16Z

@jreback I merged to master and resolved conflicts, pls take a look

TomAugspurger · 2019-06-13T20:39:52Z

I just reread through the issue, and my thinking has changed a bit.

Do we think that .str.replace('.', '...') replacing every character is actually the most useful thing for users? Or is the change primarily motivated by the inconsistency?

If we think that most users will expect .str.replace(".") to only replace periods, then perhaps we just document that we don't auto-regex single-character pat (., ^, $, |) and tell users to pass re.compile(pat) if they want that?

WillAyd · 2019-06-13T20:51:21Z

Or is the change primarily motivated by the inconsistency

Definitely inconsistency IMO. I don't see that particular operation being useful but I don't think we should be special casing either, as our general handling of regexes in str replacement ops is all over the place

TomAugspurger · 2019-06-13T21:26:01Z

What other inconsistencies do you see? FWIW, I perfectly fine with documenting that length-1 `pat` aren't auto-converted to regex, even when `regex` is true. I think the state I want to avoid is surprising future users with `.` replacing every character, just because they aren't familiar with regex. So either we document that length-1 characters are special cased, or deprecate `regex=True`. I don't have a good feeling for which is better.

…

On Thu, Jun 13, 2019 at 3:51 PM William Ayd ***@***.***> wrote: Or is the change primarily motivated by the inconsistency Definitely inconsistency IMO. I don't see that particular operation being useful but I don't think we should be special casing either, as our general handling of regexes in str replacement ops is all over the place — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24935?email_source=notifications&email_token=AAKAOIRDAVDBAIYQXJOCRPLP2KXNDA5CNFSM4GSNWKT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXU727Y#issuecomment-501874047>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOISY433NZDDETRU4BRTP2KXNDANCNFSM4GSNWKTQ> .

WillAyd · 2019-06-13T22:17:19Z

I wouldn't be opposed to really doing nothing on this atm. The behavior can definitely be surprising as noted by the OP of the original issue, but to your point I don't see why the strictly standard behavior is useful either.

May be worth just punting this for a more comprehensive alignment of the regex keywords across string ops if it makes sense to tackle then

TomAugspurger · 2019-06-17T20:51:01Z

By "doing nothing" do you include "document the current behavior of special casing length-1 patterns"? If so, then I agree that's best for now :)

…

On Thu, Jun 13, 2019 at 5:17 PM William Ayd ***@***.***> wrote: I wouldn't be opposed to really doing nothing on this atm. The behavior can definitely be surprising as noted by the OP of the original issue, but to your point I don't see why the strictly standard behavior is useful either. May be worth just punting this for a more comprehensive alignment of the regex keywords across string ops if it makes sense to tackle then — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24935?email_source=notifications&email_token=AAKAOISFQDIHWOVNJW3YD3LP2LBPNA5CNFSM4GSNWKT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXVF47I#issuecomment-501898877>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIW6JF43GV6CMPMKA3LP2LBPNANCNFSM4GSNWKTQ> .

WillAyd · 2019-06-17T22:30:49Z

Agreed - @charlesdong1991 make sense to you?

charlesdong1991 · 2019-06-17T23:29:58Z

ok, it does.

charlesdong1991 changed the title ~~fix str.replace('.','') should replace every character~~ BUG: fix str.replace('.','') should replace every character Jan 25, 2019

charlesdong1991 force-pushed the issue_24804 branch 2 times, most recently from 8b8c210 to 4b9672a Compare January 26, 2019 08:17

jreback added the Strings String extension data type and string data label Jan 26, 2019

jreback requested changes Jan 26, 2019

View reviewed changes

TomAugspurger reviewed Jan 28, 2019

View reviewed changes

jreback mentioned this pull request Feb 11, 2019

BUG: Fix regression on DataFrame.replace for regex #25266

Merged

4 tasks

charlesdong1991 force-pushed the issue_24804 branch from 5916bc6 to f778bde Compare February 11, 2019 21:41

jreback requested changes Feb 16, 2019

View reviewed changes

charlesdong1991 closed this Feb 16, 2019

charlesdong1991 force-pushed the issue_24804 branch from 6fa8c0f to b0dac6c Compare February 16, 2019 18:28

charlesdong1991 reopened this Feb 16, 2019

charlesdong1991 force-pushed the issue_24804 branch 2 times, most recently from 7e55526 to 2c9f4fb Compare February 18, 2019 08:41

jreback requested changes Mar 3, 2019

View reviewed changes

jreback added Bug Deprecate Functionality to remove in pandas labels Mar 3, 2019

charlesdong1991 force-pushed the issue_24804 branch from 2c9f4fb to 1f97148 Compare March 3, 2019 09:20

charlesdong1991 force-pushed the issue_24804 branch from 374b1c7 to 6a643ec Compare March 31, 2019 09:53

charlesdong1991 force-pushed the issue_24804 branch from 876c93a to 0f172d8 Compare June 12, 2019 09:12

merge master

aaf9be3

charlesdong1991 force-pushed the issue_24804 branch from 55efcd9 to aaf9be3 Compare June 12, 2019 19:32

Merge remote-tracking branch 'upstream/master' into issue_24804

6267aae

charlesdong1991 closed this Jun 17, 2019

BUG: fix str.replace('.','') should replace every character #24935

BUG: fix str.replace('.','') should replace every character #24935

Conversation

charlesdong1991 commented Jan 25, 2019 • edited Loading

pep8speaks commented Jan 25, 2019 • edited Loading

Comment last updated at 2019-06-13 00:02:24 UTC

codecov bot commented Jan 25, 2019

Codecov Report

codecov bot commented Jan 25, 2019 • edited Loading

Codecov Report

jreback 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

Choose a reason for hiding this comment

TomAugspurger Jan 28, 2019 • 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

Choose a reason for hiding this comment

charlesdong1991 Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesdong1991 commented Feb 4, 2019

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesdong1991 commented Feb 16, 2019

WillAyd commented Feb 28, 2019

charlesdong1991 commented Feb 28, 2019

WillAyd commented Feb 28, 2019

TomAugspurger commented Feb 28, 2019

WillAyd commented Feb 28, 2019

TomAugspurger commented Feb 28, 2019

WillAyd commented Feb 28, 2019

TomAugspurger commented Feb 28, 2019 via email

WillAyd commented Feb 28, 2019

TomAugspurger commented Feb 28, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Mar 3, 2019

WillAyd commented Mar 3, 2019 via email

jreback commented Mar 20, 2019

charlesdong1991 commented Mar 24, 2019

jreback commented Mar 24, 2019

charlesdong1991 commented Mar 31, 2019

WillAyd commented Apr 10, 2019

TomAugspurger commented Apr 10, 2019 via email

jreback commented Apr 10, 2019

charlesdong1991 commented Apr 12, 2019

jreback commented Jun 8, 2019

charlesdong1991 commented Jun 12, 2019

TomAugspurger commented Jun 13, 2019

WillAyd commented Jun 13, 2019

TomAugspurger commented Jun 13, 2019 via email

WillAyd commented Jun 13, 2019

TomAugspurger commented Jun 17, 2019 via email

WillAyd commented Jun 17, 2019

charlesdong1991 commented Jun 17, 2019

charlesdong1991 commented Jan 25, 2019 •

edited

Loading

pep8speaks commented Jan 25, 2019 •

edited

Loading

codecov bot commented Jan 25, 2019 •

edited

Loading

TomAugspurger Jan 28, 2019 •

edited

Loading

charlesdong1991 Jan 28, 2019 •

edited

Loading