Repository: pandas-dev/pandas
Pull request: BUG: df.replace over pd.Period columns (#34871)
- [x] closes #34871
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This commit ensures that PeriodArrays return False for _can_hold_element for any element that is not a pd.Period. This prevents upstream code from casting the dtype to object. Also un-xfail test written in #34904

Problem statement:
BUG: Inconsistent behavior for df.replace over pd.Period columns
- [x] I have checked that this issue has not already been reported.

- [x] I have confirmed this bug exists on the latest version of pandas.

- [ ] (optional) I have confirmed this bug exists on the master branch of pandas.

---

#### Code Sample, a copy-pastable example

```python
import pandas as pd

df = pd.DataFrame({
    'Int': [1, 2, 3],
    'Flo': [1.0, 6.0, 1.0],
    'Per': [pd.Period('2020-01')]*3,
    'Tim': [pd.Timestamp('2020-01')]*3
})

# TypeError: 'value' should be a 'Period', 'NaT', or array of those. Got 'float' instead.
df.replace(1.0, 0.0)

# Works
df[['Int', 'Flo', 'Tim']].replace(1.0, 0.0)

# Works
df['Per'].replace(1.0, pd.Period('2020-02'))

```

#### Problem description

`df.replace(1.0, 0.0)` fails if any columns of `df` are Period types. The error is saying that 0.0 is an invalid value to put in a Pandas Period column. This should never be an issue though, 1.0 will never exist in a Pandas Period column, so nothing will ever be replaced for 0.0.

#### Expected Output

The expected behavior would be consistent with the `df[['Int', 'Flo', 'Tim']].replace(1.0, 0.0)` in the example above. This does work, the 'Flo' column has all of it's 1.0s replaced with 0.0s and the 'Int' and 'Tim' columns (which are not Float types) remain unaffected. I would expect that a Pandas Period column would also be skipped in the same way. 

#### Output of ``pd.show_versions()``

<details>

INSTALLED VERSIONS
------------------
commit           : None
python           : 3.7.7.final.0
python-bits      : 64
OS               : Darwin
OS-release       : 19.5.0
machine          : x86_64
processor        : i386
byteorder        : little
LC_ALL           : None
LANG             : en_US.UTF-8
LOCALE           : en_US.UTF-8

pandas           : 1.0.4
numpy            : 1.18.5
pytz             : 2020.1
dateutil         : 2.8.0
pip              : 20.1.1
setuptools       : 47.3.0.post20200616
Cython           : None
pytest           : 4.4.1
hypothesis       : None
sphinx           : 3.1.1
blosc            : None
feather          : None
xlsxwriter       : 1.2.9
lxml.etree       : 4.5.1
html5lib         : None
pymysql          : None
psycopg2         : None
jinja2           : 2.11.2
IPython          : 7.15.0
pandas_datareader: None
bs4              : None
bottleneck       : None
fastparquet      : None
gcsfs            : None
lxml.etree       : 4.5.1
matplotlib       : 3.2.1
numexpr          : None
odfpy            : None
openpyxl         : None
pandas_gbq       : None
pyarrow          : None
pytables         : None
pytest           : 4.4.1
pyxlsb           : None
s3fs             : None
scipy            : 1.4.1
sqlalchemy       : None
tables           : None
tabulate         : None
xarray           : None
xlrd             : 1.2.0
xlwt             : None
xlsxwriter       : 1.2.9
numba            : None

</details>

Hints from issue discussion:
This looks to be fixed on master (but could warrant a test if there isn't one already). Interesting though that the 1 in the first column appears not to be replaced.

```python
>>> import pandas as pd
>>> 
>>> df = pd.DataFrame({
...     'Int': [1, 2, 3],
...     'Flo': [1.0, 6.0, 1.0],
...     'Per': [pd.Period('2020-01')]*3,
...     'Tim': [pd.Timestamp('2020-01')]*3
... })
>>> print(df.replace(1.0, 0.0))
   Int  Flo      Per        Tim
0    1  0.0  2020-01 2020-01-01
1    2  6.0  2020-01 2020-01-01
2    3  0.0  2020-01 2020-01-01
>>> print(pd.__version__)
1.1.0.dev0+1900.g80ba4c429
```
I just realized that Pandas 1.0.5 was released, I was running this on version 1.0.4. I believe everything is fixed now!
take
Hi all,

I can confirm that this failed in 1.0.4 and indeed does not raise an exception anymore in 1.10. 

However, the new behavior in 1.10 is still not what I would expect it to be. Concretely, calling df.replace(1.0, 0.0) on a dataframe with a Period column now has the following effects:

* the values in a Period column are now left unchanged
* but the dtype of that same column is changed from Period to Object

I think a user would be surprised by this change in dtype (I certainly am). This also makes tm.assert_frame_equal() fail when trying to write a test for this.

For now I have created a tiny regression test that makes sure that calling df.replace() does not raise an exception anymore, see this pull request: https://github.com/pandas-dev/pandas/pull/34904

I think this issue should stay open and df.replace() should get changed to return a Period column in the scenario described. That's a bit beyond my current insights into the Pandas code, it'd be great if someone else could have a look at that.

Minimal code to reproduce (adapted from original submitter):
```python
df = pd.DataFrame({"Per": [pd.Period("2020-01")] * 3})
new_df = df.replace(1.0, 0.0)

# this is 'Period'
df.Per.dtype
# this is 'Object' but should also be 'Period'
new_df.Per.dtype
```

Unit test for desired behavior implemented here: https://github.com/pandas-dev/pandas/pull/34904

Will have a look at underlying issue causing modification of dtype from period to object
> Will have a look at underlying issue causing modification of dtype from period to object

I believe this is the same problem described in #35268.

Commit message: BUG: df.replace over pd.Period columns (#34871)

Patch to review:
diff --git a/pandas/core/internals/blocks.py b/pandas/core/internals/blocks.py
index 09f276be7d64a..c14b61c8b621d 100644
--- a/pandas/core/internals/blocks.py
+++ b/pandas/core/internals/blocks.py
@@ -10,6 +10,7 @@
 import pandas._libs.internals as libinternals
 from pandas._libs.internals import BlockPlacement
 from pandas._libs.tslibs import conversion
+from pandas._libs.tslibs.period import Period
 from pandas._libs.tslibs.timezones import tz_compare
 from pandas._typing import ArrayLike, Scalar
 from pandas.util._validators import validate_bool_kwarg
@@ -1975,6 +1976,18 @@ def external_values(self):
         return self.values.astype(object)
 
 
+class PeriodExtensionBlock(ObjectValuesExtensionBlock):
+    """
+    Used by PeriodArray to ensure proper type conversions
+    """
+
+    def _can_hold_element(self, element: Any) -> bool:
+        tipo = maybe_infer_dtype_type(element)
+        if tipo is not None:
+            return issubclass(tipo.type, Period)
+        return isinstance(element, Period)
+
+
 class NumericBlock(Block):
     __slots__ = ()
     is_numeric = True
@@ -2747,8 +2760,10 @@ def get_block_type(values, dtype=None):
         cls = DatetimeBlock
     elif is_datetime64tz_dtype(values.dtype):
         cls = DatetimeTZBlock
-    elif is_interval_dtype(dtype) or is_period_dtype(dtype):
+    elif is_interval_dtype(dtype):
         cls = ObjectValuesExtensionBlock
+    elif is_period_dtype(dtype):
+        cls = PeriodExtensionBlock
     elif is_extension_array_dtype(values.dtype):
         cls = ExtensionBlock
     elif issubclass(vtype, np.floating):
diff --git a/pandas/core/internals/managers.py b/pandas/core/internals/managers.py
index f2480adce89b4..acfddf3f784b4 100644
--- a/pandas/core/internals/managers.py
+++ b/pandas/core/internals/managers.py
@@ -47,6 +47,7 @@
     DatetimeTZBlock,
     ExtensionBlock,
     ObjectValuesExtensionBlock,
+    PeriodExtensionBlock,
     extend_blocks,
     get_block_type,
     make_block,
@@ -1743,6 +1744,14 @@ def form_blocks(arrays, names: Index, axes) -> List[Block]:
 
         blocks.extend(external_blocks)
 
+    if len(items_dict["PeriodExtensionBlock"]):
+        external_blocks = [
+            make_block(array, klass=PeriodExtensionBlock, placement=i)
+            for i, _, array in items_dict["PeriodExtensionBlock"]
+        ]
+
+        blocks.extend(external_blocks)
+
     if len(extra_locs):
         shape = (len(extra_locs),) + tuple(len(x) for x in axes[1:])
 
diff --git a/pandas/tests/frame/methods/test_replace.py b/pandas/tests/frame/methods/test_replace.py
index a9cf840470ae0..5345d08c1edf0 100644
--- a/pandas/tests/frame/methods/test_replace.py
+++ b/pandas/tests/frame/methods/test_replace.py
@@ -1517,9 +1517,6 @@ def test_replace_with_duplicate_columns(self, replacement):
 
         tm.assert_frame_equal(result, expected)
 
-    @pytest.mark.xfail(
-        reason="replace() changes dtype from period to object, see GH34871", strict=True
-    )
     def test_replace_period_ignore_float(self):
         """
         Regression test for GH#34871: if df.replace(1.0, 0.0) is called on a df


Code review comments:
diff_hunk: @@ -10,6 +10,7 @@
 import pandas._libs.internals as libinternals
 from pandas._libs.internals import BlockPlacement
 from pandas._libs.tslibs import conversion
+from pandas._libs.tslibs.period import Period
path: pandas/core/internals/blocks.py
text: @user1:
let's just fold this import into the previous line and get it from tslibs