Skip to content

REGR: Don't ignore compiled patterns in replace #35697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 17, 2020
Merged

REGR: Don't ignore compiled patterns in replace #35697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d21e4f0
REGR: Don't ignore compiled patterns in replace
dsaxton Aug 13, 2020
5006980
Type
dsaxton Aug 13, 2020
40bb67c
Revert "Type"
dsaxton Aug 13, 2020
a05f3b2
Type
dsaxton Aug 15, 2020
3d5c167
Merge remote-tracking branch 'upstream/master' into compiled-regex-re…
dsaxton Aug 15, 2020
01e90cb
Type and docstring
dsaxton Aug 15, 2020
264d17d
Fix
dsaxton Aug 16, 2020
ae6f53f
Revert "Fix"
dsaxton Aug 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.1.1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Fixed regressions
- Fixed regression in :meth:`DataFrame.reset_index` would raise a ``ValueError`` on empty :class:`DataFrame` with a :class:`MultiIndex` with a ``datetime64`` dtype level (:issue:`35606`, :issue:`35657`)
- Fixed regression where :meth:`DataFrame.merge_asof` would raise a ``UnboundLocalError`` when ``left_index`` , ``right_index`` and ``tolerance`` were set (:issue:`35558`)
- Fixed regression in ``.groupby(..).rolling(..)`` where a custom ``BaseIndexer`` would be ignored (:issue:`35557`)
- Fixed regression in :meth:`DataFrame.replace` and :meth:`Series.replace` where compiled regular expressions would be ignored during replacement (:issue:`35680`)
- Fixed regression in :meth:`~pandas.core.groupby.DataFrameGroupBy.agg` where a list of functions would produce the wrong results if at least one of the functions did not aggregate. (:issue:`35490`)

.. ---------------------------------------------------------------------------
Expand Down
23 changes: 18 additions & 5 deletions pandas/core/internals/managers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,17 @@
import itertools
import operator
import re
from typing import DefaultDict, Dict, List, Optional, Sequence, Tuple, TypeVar, Union
from typing import (
DefaultDict,
Dict,
List,
Optional,
Pattern,
Sequence,
Tuple,
TypeVar,
Union,
)
import warnings

import numpy as np
Expand Down Expand Up @@ -1907,7 +1917,10 @@ def _merge_blocks(


def _compare_or_regex_search(
a: ArrayLike, b: Scalar, regex: bool = False, mask: Optional[ArrayLike] = None
a: ArrayLike,
b: Union[Scalar, Pattern],
regex: bool = False,
mask: Optional[ArrayLike] = None,
) -> Union[ArrayLike, bool]:
"""
Compare two array_like inputs of the same shape or two scalar values
Expand All @@ -1918,7 +1931,7 @@ def _compare_or_regex_search(
Parameters
----------
a : array_like
b : scalar
b : scalar or regex pattern
regex : bool, default False
mask : array_like or None (default)

Expand All @@ -1928,7 +1941,7 @@ def _compare_or_regex_search(
"""

def _check_comparison_types(
result: Union[ArrayLike, bool], a: ArrayLike, b: Scalar,
result: Union[ArrayLike, bool], a: ArrayLike, b: Union[Scalar, Pattern],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is a Pattern ever passed through to _check_comparison_types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's just for the error message

):
"""
Raises an error if the two arrays (a,b) cannot be compared.
Expand All @@ -1949,7 +1962,7 @@ def _check_comparison_types(
else:
op = np.vectorize(
lambda x: bool(re.search(b, x))
if isinstance(x, str) and isinstance(b, str)
if isinstance(x, str) and isinstance(b, (str, Pattern))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b is typed as Scalar, where
Scalar = Union[PythonScalar, PandasScalar]

but b resolves to Union[builtins.str, builtins.float, Any] due to unresolved imports and hence mypy is green

but should probably update types and docstring anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @simonjayhawkins. Would the proper type for b be Union[Scalar, Pattern], or should PythonScalar be extended to include Pattern?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep PythonScalar unchanged for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Odd that b is typed as scalar but there's an isinstance check for np.ndarray inside the function, maybe that's not needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the docstring also implies b could be an array. I guess could add ArrayLike while here or leave. either works for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add it. Actually a can also be a scalar so the type shouldn't only be ArrayLike I don't think:

In [17]: b
Out[17]: array(['a', 'a', 'a'], dtype='<U1')

In [18]: _compare_or_regex_search("a", b)
Out[18]: array([ True,  True,  True])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the types were originally this way when added, see also #32890 (comment), but changed in #33598

maybe best to keep the changes here minimal for now as this PR is being backported

(and separate PR for ensuring the types are correct and consistent with docstrings.)

else False
)

Expand Down
8 changes: 8 additions & 0 deletions pandas/tests/frame/methods/test_replace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1573,3 +1573,11 @@ def test_replace_dict_category_type(self, input_category_df, expected_category_d
result = input_df.replace({"a": "z", "obj1": "obj9", "cat1": "catX"})

tm.assert_frame_equal(result, expected)

def test_replace_with_compiled_regex(self):
# https://github.com/pandas-dev/pandas/issues/35680
df = pd.DataFrame(["a", "b", "c"])
regex = re.compile("^a$")
result = df.replace({regex: "z"}, regex=True)
expected = pd.DataFrame(["z", "b", "c"])
tm.assert_frame_equal(result, expected)
10 changes: 10 additions & 0 deletions pandas/tests/series/methods/test_replace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import re

import numpy as np
import pytest

Expand Down Expand Up @@ -415,3 +417,11 @@ def test_replace_extension_other(self):
# https://github.com/pandas-dev/pandas/issues/34530
ser = pd.Series(pd.array([1, 2, 3], dtype="Int64"))
ser.replace("", "") # no exception

def test_replace_with_compiled_regex(self):
# https://github.com/pandas-dev/pandas/issues/35680
s = pd.Series(["a", "b", "c"])
regex = re.compile("^a$")
result = s.replace({regex: "z"}, regex=True)
expected = pd.Series(["z", "b", "c"])
tm.assert_series_equal(result, expected)