Skip to content

REGR: Replace changes the dtype of other columns #46636

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 4 commits into from
May 25, 2022
Merged

REGR: Replace changes the dtype of other columns #46636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2d99ba
REGR: Replace changes the dtype of other columns
simonjayhawkins Apr 4, 2022
c0cb9ed
Merge remote-tracking branch 'upstream/main' into replace-regression
simonjayhawkins Apr 12, 2022
4aaa243
copy if not inplace
simonjayhawkins Apr 12, 2022
79cc4d2
Merge branch 'main' into replace-regression
simonjayhawkins May 15, 2022
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
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.4.3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ including other versions of pandas.

Fixed regressions
~~~~~~~~~~~~~~~~~
-
- Fixed regression in :meth:`DataFrame.replace` when the replacement value was explicitly ``None`` when passed in a dictionary to ``to_replace`` also casting other columns to object dtype even when there were no values to replace (:issue:`46634`)
-

.. ---------------------------------------------------------------------------
Expand Down
14 changes: 8 additions & 6 deletions pandas/core/internals/blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,12 +781,14 @@ def _replace_coerce(
)
else:
if value is None:
# gh-45601, gh-45836
nb = self.astype(np.dtype(object), copy=False)
if nb is self and not inplace:
nb = nb.copy()
putmask_inplace(nb.values, mask, value)
return [nb]
# gh-45601, gh-45836, gh-46634
if mask.any():
nb = self.astype(np.dtype(object), copy=False)
if nb is self and not inplace:
nb = nb.copy()
putmask_inplace(nb.values, mask, value)
return [nb]
return [self]
Copy link
Member

Choose a reason for hiding this comment

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

probably need self.copy() if not inplace?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, in #46404 (the cause of the latest regression), I reverted some of the code change in #44940 (the cause of the previous regression). In this PR I have reverted some more of the changes in #44940 that obviously (now!) should also have been included in the previous regression fix. I don't see the original code making a copy if not mask.any().

but yes, in #44940 the code path was changed to dispatch to Block.replace which does make a copy even if the replace is a no-op. So as a pure regression fix we could keep this as is, but also happy to do a copy for consistency with other replacements in 1.4.x

my biggest concern (need to check this though) is categorical since the special casing was also moved in #44940.

I will mark this a draft for now, (till we get closer to the next release) and see if we can get the issues on master sorted so that we can dispatch to Block.replace when the replacement value is None. (doing that might end up being difficult to backport though)

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty actively trying to dis-engage, not gonna fuss about it either way. i trust your judgment here.

Copy link
Member Author

@simonjayhawkins simonjayhawkins Apr 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

my biggest concern (need to check this though) is categorical since the special casing was also moved in #44940.

There is an issue raised for the categorical issue #46672, so can keep that out of this PR for now.

I will mark this a draft for now, (till we get closer to the next release) and see if we can get the issues on master sorted so that we can dispatch to Block.replace when the replacement value is None. (doing that might end up being difficult to backport though)

I guess no harm in getting this bit sorted and then doing that as a separate PR. (maybe after #46672 is sorted).

return self.replace(
to_replace=to_replace, value=value, inplace=inplace, mask=mask
)
Expand Down
19 changes: 19 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 @@ -675,6 +675,25 @@ def test_replace_NAT_with_None(self):
expected = DataFrame([None, None])
tm.assert_frame_equal(result, expected)

def test_replace_with_None_keeps_categorical(self):
# gh-46634
cat_series = Series(["b", "b", "b", "d"], dtype="category")
df = DataFrame(
{
"id": Series([5, 4, 3, 2], dtype="float64"),
"col": cat_series,
}
)
result = df.replace({3: None})

expected = DataFrame(
{
"id": Series([5.0, 4.0, None, 2.0], dtype="object"),
"col": cat_series,
}
)
tm.assert_frame_equal(result, expected)

def test_replace_value_is_none(self, datetime_frame):
orig_value = datetime_frame.iloc[0, 0]
orig2 = datetime_frame.iloc[1, 0]
Expand Down