Repository: pandas-dev/pandas
Pull request: API: replace() should raise an exception if invalid argument is given
- [x] closes #18634
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

I used `is_scalar()` instead of `is_numeric()` to allow data structures like pd.Timestamp to be used in `replace()`. See tests in pandas/tests/frame/methods/test_replace.py, pandas/tests/series/methods/test_replace.py for examples.

Problem statement:
Series.replace() should raise an exception if invalid argument is given
Series.replace() should potentially raise an exception if an invalid argument is given? I am new to pandas and this is more of a question. **This is a user error on my part, but possible improvement to replace.**

```python
df = pd.DataFrame(
{
    'one': ['1','1 ','10'],
    'two': ['1 ', '20 ', '30 ']
})

# creates
     one  two
0    1    1
1    1    20
2    10    30

# I intentionally added a space in df.one. So df.one.value_counts() results in
1     1
10    1
1     1

# I want to strip the spaces around all values in df.one 
# so that value_counts() only has 1 and 10 with values 2 and 1 respectively. 
# The following does not work.

df.one.replace(lambda x: x.strip(), inplace=True)

```
#### Problem description

Series.replace() accepts `"str, regex, list, dict, Series, numeric, or None"`. So I understand why the above does not work. But as @toobaz pointed out on gitter, when a lambda is passed, replace should probably raise an error?

#### Expected Output
Raises TypeError

#### Output of ``pd.show_versions()``

<details>

[paste the output of ``pd.show_versions()`` here below this line]

INSTALLED VERSIONS
------------------
commit: None
python: 3.6.1.final.0
python-bits: 64
OS: Darwin
OS-release: 16.7.0
machine: x86_64
processor: i386
byteorder: little
LC_ALL: None
LANG: en_US.UTF-8
LOCALE: en_US.UTF-8

pandas: 0.20.3
pytest: None
pip: 9.0.1
setuptools: 36.3.0
Cython: None
numpy: 1.13.1
scipy: 0.19.1
xarray: None
IPython: 6.1.0
sphinx: 1.6.3
patsy: None
dateutil: 2.6.1
pytz: 2017.2
blosc: None
bottleneck: None
tables: None
numexpr: 2.6.2
feather: None
matplotlib: 2.0.2
openpyxl: 2.4.8
xlrd: 1.0.0
xlwt: None
xlsxwriter: None
lxml: 3.8.0
bs4: None
html5lib: 0.999999999
sqlalchemy: 1.1.14
pymysql: 0.7.11.None
psycopg2: None
jinja2: 2.8.1
s3fs: None
pandas_gbq: None
pandas_datareader: None
</details>

Hints from issue discussion:
> replace should probably raise an error?

@lidderupk : That makes sense to me.  PR to implement is more than welcome!
@gfyoung: forked the codebase and reading "Contributing to Pandas" now. Is this a beginner friendly PR? I would like to contribute if it is. Thank you.
This is one is not hard, but if you want an even more beginner friendly one, take a look for instance at #18589 . Or you can get the list of [easy issues](https://github.com/pandas-dev/pandas/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3A%22Difficulty+Novice%22+label%3A%22Effort+Low%22) and pick the one you prefer.
@lidderupk : There is very much do-able as a beginner.  Feel free to ask questions if you're unclear on anything.  If you get stuck, submit what you have as a PR, and we can help you out.

Commit message: API: another attempt to fix docs.

Patch to review:
diff --git a/doc/source/whatsnew/v1.1.0.rst b/doc/source/whatsnew/v1.1.0.rst
index 13827e8fc4c33..1435e0901351f 100644
--- a/doc/source/whatsnew/v1.1.0.rst
+++ b/doc/source/whatsnew/v1.1.0.rst
@@ -56,6 +56,7 @@ Other API changes
 - :meth:`Series.describe` will now show distribution percentiles for ``datetime`` dtypes, statistics ``first`` and ``last``
   will now be ``min`` and ``max`` to match with numeric dtypes in :meth:`DataFrame.describe` (:issue:`30164`)
 - :meth:`Groupby.groups` now returns an abbreviated representation when called on large dataframes (:issue:`1135`)
+- :meth:`DataFrame.replace` and :meth:`Series.replace` will raise a ``TypeError`` if ``to_replace`` is not an expected type. Previously the ``replace`` would fail silently (:issue:`18634`)
 
 Backwards incompatible API changes
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/pandas/core/generic.py b/pandas/core/generic.py
index 1a16f8792e9e7..4c704e71b4538 100644
--- a/pandas/core/generic.py
+++ b/pandas/core/generic.py
@@ -6189,7 +6189,11 @@ def bfill(
         AssertionError
             * If `regex` is not a ``bool`` and `to_replace` is not
               ``None``.
+
         TypeError
+            * If `to_replace` is not any of the expected types
+              (``str``, `regex`, ``list``, ``dict``, ``Series``, ``int``,
+              ``float`` or ``None``)
             * If `to_replace` is a ``dict`` and `value` is not a ``list``,
               ``dict``, ``ndarray``, or ``Series``
             * If `to_replace` is ``None`` and `regex` is not compilable
@@ -6198,6 +6202,7 @@ def bfill(
             * When replacing multiple ``bool`` or ``datetime64`` objects and
               the arguments to `to_replace` does not match the type of the
               value being replaced
+
         ValueError
             * If a ``list`` or an ``ndarray`` is passed to `to_replace` and
               `value` but they are not the same length.
@@ -6393,6 +6398,19 @@ def replace(
         regex=False,
         method="pad",
     ):
+        if not (
+            is_scalar(to_replace)
+            or isinstance(to_replace, pd.Series)
+            or is_re_compilable(to_replace)
+            or is_list_like(to_replace)
+            or is_dict_like(to_replace)
+        ):
+            raise TypeError(
+                f"Expecting 'to_replace' to be str, regex, list, dict, Series, "
+                f"int, float or None, got invalid type "
+                f"{repr(type(to_replace).__name__)}"
+            )
+
         inplace = validate_bool_kwarg(inplace, "inplace")
         if not is_bool(regex) and to_replace is not None:
             raise AssertionError("'to_replace' must be 'None' if 'regex' is not a bool")
diff --git a/pandas/tests/frame/methods/test_replace.py b/pandas/tests/frame/methods/test_replace.py
index 92b74c4409d7d..b225e20e42bdc 100644
--- a/pandas/tests/frame/methods/test_replace.py
+++ b/pandas/tests/frame/methods/test_replace.py
@@ -1363,3 +1363,27 @@ def test_replace_after_convert_dtypes(self):
         result = df.replace(1, 10)
         expected = pd.DataFrame({"grp": [10, 2, 3, 4, 5]}, dtype="Int64")
         tm.assert_frame_equal(result, expected)
+
+    @pytest.mark.parametrize(
+        "df, to_replace",
+        [
+            (
+                {"one": ["a", "b ", "c"], "two": ["d ", "e ", "f "]},
+                lambda x: x.strip(),
+            ),
+            (
+                {"one": ["a", "b ", "c"], "two": ["d ", "e ", "f "]},
+                type("callable_object", (object,), dict(__call__=lambda x: x.strip())),
+            ),
+        ],
+    )
+    def test_replace_invalid_to_replace(self, df, to_replace):
+        # GH 18634
+        # API: replace() should raise an exception if invalid argument is given
+        df = pd.DataFrame(df)
+        msg = (
+            r"Expecting 'to_replace' to be str, regex, list, dict, Series, "
+            r"int, float or None, got invalid type.*"
+        )
+        with pytest.raises(TypeError, match=msg):
+            df.replace(to_replace)
diff --git a/pandas/tests/series/methods/test_replace.py b/pandas/tests/series/methods/test_replace.py
index 770ad38b0215e..7112938cc4f43 100644
--- a/pandas/tests/series/methods/test_replace.py
+++ b/pandas/tests/series/methods/test_replace.py
@@ -362,3 +362,24 @@ def test_replace_no_cast(self, ser, exp):
         expected = pd.Series(exp)
 
         tm.assert_series_equal(result, expected)
+
+    @pytest.mark.parametrize(
+        "ser, to_replace",
+        [
+            (["a", "b", "c "], lambda x: x.strip()),
+            (
+                ["a", "b", "c "],
+                type("callable_object", (object,), dict(__call__=lambda x: x.strip())),
+            ),
+        ],
+    )
+    def test_replace_invalid_to_replace(self, ser, to_replace):
+        # GH 18634
+        # API: replace() should raise an exception if invalid argument is given
+        series = pd.Series(ser)
+        msg = (
+            r"Expecting 'to_replace' to be str, regex, list, dict, Series, "
+            r"int, float or None, got invalid type.*"
+        )
+        with pytest.raises(TypeError, match=msg):
+            series.replace(to_replace)


Code review comments:
diff_hunk: @@ -1363,3 +1363,27 @@ def test_replace_after_convert_dtypes(self):
         result = df.replace(1, 10)
         expected = pd.DataFrame({"grp": [10, 2, 3, 4, 5]}, dtype="Int64")
         tm.assert_frame_equal(result, expected)
+
+    @pytest.mark.parametrize(
+        "df, to_replace",
+        [
+            (
+                {"one": ["a", "b ", "c"], "two": ["d ", "e ", "f "]},
+                lambda x: x.strip(),
+            ),
+            (
+                {"one": ["a", "b ", "c"], "two": ["d ", "e ", "f "]},
+                type("callable_object", (object,), dict(__call__=lambda x: x.strip())),
path: pandas/tests/frame/methods/test_replace.py
text: @user1:
This is practically the same as the lambda right? I think just the former is fine

@author:
Yes, it is. I can just move the df and lambda to the body.

diff_hunk: @@ -6393,6 +6398,19 @@ def replace(
         regex=False,
         method="pad",
     ):
+        if not (
+            is_scalar(to_replace)
+            or isinstance(to_replace, pd.Series)
+            or is_re_compilable(to_replace)
+            or is_list_like(to_replace)
+            or is_dict_like(to_replace)
+        ):
+            raise TypeError(
+                f"Expecting 'to_replace' to be str, regex, list, dict, Series, "
+                f"int, float or None, got invalid type "
path: pandas/core/generic.py
text: @user1:
```suggestion
                "int, float or None, got invalid type "
```

diff_hunk: @@ -6393,6 +6398,19 @@ def replace(
         regex=False,
         method="pad",
     ):
+        if not (
+            is_scalar(to_replace)
+            or isinstance(to_replace, pd.Series)
+            or is_re_compilable(to_replace)
+            or is_list_like(to_replace)
+            or is_dict_like(to_replace)
path: pandas/core/generic.py
text: @user1:
`is_list_like` actually will cover the dict case, so don't need this line

@author:
Removed, and thank you for the PR feedback!

diff_hunk: @@ -1363,3 +1363,27 @@ def test_replace_after_convert_dtypes(self):
         result = df.replace(1, 10)
         expected = pd.DataFrame({"grp": [10, 2, 3, 4, 5]}, dtype="Int64")
         tm.assert_frame_equal(result, expected)
+
+    @pytest.mark.parametrize(
+        "df, to_replace",
+        [
+            (
+                {"one": ["a", "b ", "c"], "two": ["d ", "e ", "f "]},
path: pandas/tests/frame/methods/test_replace.py
text: @user1:
Since `df` is the same all of the time no need to parametrize; just place in the body

diff_hunk: @@ -6393,6 +6398,19 @@ def replace(
         regex=False,
         method="pad",
     ):
+        if not (
+            is_scalar(to_replace)
+            or isinstance(to_replace, pd.Series)
+            or is_re_compilable(to_replace)
+            or is_list_like(to_replace)
+            or is_dict_like(to_replace)
+        ):
+            raise TypeError(
+                f"Expecting 'to_replace' to be str, regex, list, dict, Series, "
path: pandas/core/generic.py
text: @user1:
```suggestion
                "Expecting 'to_replace' to be str, regex, list, dict, Series, "
```

Minor but this isn't necessary