ENH: str.replace accepts a compiled expression
State: closed

.str.replace now accepts a compiled regular expression.

See #15446

 - [x] closes #15446
 - [x] tests added / passed
 - [x] passes ``git diff upstream/master | flake8 --diff``
 - [x] whatsnew entry


Labels:
name: Enhancement
name: Strings
description: String extension data type and string data

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15456?src=pr&el=h1) Report
> Merging [#15456](https://codecov.io/gh/pandas-dev/pandas/pull/15456?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0b776806b78421e377bb6c305d1e3a752f24e358?src=pr&el=desc) will **decrease** coverage by `-0.03%`.
> The diff coverage is `100%`.


```diff
@@            Coverage Diff             @@
##           master   #15456      +/-   ##
==========================================
- Coverage   91.05%   91.03%   -0.03%     
==========================================
  Files         137      137              
  Lines       49285    49290       +5     
==========================================
- Hits        44878    44872       -6     
- Misses       4407     4418      +11
```


| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/15456?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/compare/0b776806b78421e377bb6c305d1e3a752f24e358...8f251456342e03e59d0d4fb34d4cb15c2bc2fb23?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `98.49% <100%> (+0.01%)` | :white_check_mark: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/compare/0b776806b78421e377bb6c305d1e3a752f24e358...8f251456342e03e59d0d4fb34d4cb15c2bc2fb23?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :x: |
| [pandas/tools/merge.py](https://codecov.io/gh/pandas-dev/pandas/compare/0b776806b78421e377bb6c305d1e3a752f24e358...8f251456342e03e59d0d4fb34d4cb15c2bc2fb23?src=pr&el=tree#diff-cGFuZGFzL3Rvb2xzL21lcmdlLnB5) | `91.78% <0%> (-0.35%)` | :x: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/compare/0b776806b78421e377bb6c305d1e3a752f24e358...8f251456342e03e59d0d4fb34d4cb15c2bc2fb23?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.83% <0%> (-0.1%)` | :x: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15456?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15456?src=pr&el=footer). Last update [0b77680...624637f](https://codecov.io/gh/pandas-dev/pandas/compare/0b776806b78421e377bb6c305d1e3a752f24e358...624637f67528fb9f85e2d26e5faa2ddcc0983f22?el=footer&src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
needs tests
Tests and documentation added. Tests passing.
Thanks for the feedback. 

I like your idea of **using a partial**; and I've implemented it in a revision that simplifies the pattern/compiled-regex case to one `lambda` using a `partial`. The `re` package will **raise an error** if `case` or `flags` are provided. Please let me know what you think.

Regarding **testing for a compiled regex**, `python/cpython/re` package tests in a similar way (see code below). Let me know if you think we should test differently.

```python
_pattern_type = type(sre_compile.compile("", 0))
...
def _compile(pattern, flags):
...
    if isinstance(pattern, _pattern_type):
```

I also made the document changes. Thanks for catching that.
Actually, I just realized that my use of `partial` is not necessary. I revised to use a simple `lambda` with `re.sub`. I'll let the tests finish before pinging back.
Tests/checks passing.

@jreback - please review when you have time. Key items to review:
- are we comfortable with checking if `pat` is a compiled regex using `type` / `isinstance`? This is how `re` checks to see if a pattern is a compiled regex.
- can we let `re` package raise errors when `flags` is provided with a compiled pattern (as is the default behavior for `re.sub`), or should `pandas` raise its own error?
Thanks for your review. Code is updated, will let it test overnight.

FYI - `flags` must be an `int` for Python 2.7's `re.sub` (i.e. `flags=None` will raise an error). I added a `compat.PY2` test to set `flags=0` when `pat` is a compiled regex. Not sure if there is a better pattern for that.
just some very minor documentation comments. Also can you add an example in ``test.rst`` (and have it say versionadded 0.20.0 as well.

@jorisvandenbossche 
I implemented changes from @jorisvandenbossche and @jreback. Thanks for the comments and discussion. Let me know if you have any other feedback.
Interested in what @jorisvandenbossche thinks about @jreback's suggestion  https://github.com/pandas-dev/pandas/pull/15456#discussion_r104260073, which is implemented in the current version; i.e., use default argument `case=object` to make it truthy.
@rouzazari ok, why don;t you revert to the previous logic where ``case=None, flags=0`` and consistent doc-strings.
All sounds good to me. Pushed approved changes and reverting back to `case=None, flags=0`. 

I'm also adding an additional test to `test_strings.py` (`case=True` with compiled regex should throw an error). I was not testing for this before, but should have been.
lgtm. ping on green.
can u add an asv for that case with re.sub?
I'm not too familiar with asv yet but it seems straight forward and worth learning. 

Just to make sure I understand, are you suggesting that I test `re.sub` versus `pattern.sub` (where `pattern` is a compiled regex) using asv? 

Looks like there is a file to step from (`asv_bench/benchmarks/strings.py`) with a `test_replace` benchmark.
> Just to make sure I understand, are you suggesting that I test re.sub versus pattern.sub (where pattern is a compiled regex) using asv?

No, the idea is to have a test that uses `str.replace` itself that you can benchmark, and eg in this case could detect a slowdown if you change the implementation from using pattern.sub to re.sub(patters, ..). 

> Looks like there is a file to step from (asv_bench/benchmarks/strings.py) with a test_replace benchmark.

There is indeed already a test, so no need to add one I think. 
You can try to run this test to see of there is any performance impact of this PR (http://pandas.pydata.org/pandas-docs/stable/contributing.html#running-the-performance-test-suite), although I don't expect that for this PR (now that you used the compiling).

@rouzazari Thanks a lot!
Great! I enjoyed working on this with both of you. Thanks for your help along the way.

Is it worth it to add a similar enhancement to other pattern-matching functions?
@rouzazari you too!

sure, why don't you open an issue with an example of what needs to change (then of course a PR is welcome)