Skip to content

BUG: update should try harder to preserve dtypes #4094 #40219

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

Closed
wants to merge 7 commits into from
Closed

BUG: update should try harder to preserve dtypes #4094 #40219

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2dc75cc
BUG GH4094 update should try harder to preserve dtypes
Mar 4, 2021
a2109bb
DOC GH4094 add whatsnew text
Mar 4, 2021
da88edf
CLN resolve inconsistent use of pandas namespace in tests
Mar 4, 2021
5bf17b5
TST GH4094 Remove limitation on endianness
Mar 4, 2021
62d8b0e
DOC GH4094 Move bugfix documentation to different version
Mar 4, 2021
7386a1f
TST GH4094 Use fixtures instead of hypothesis package
Mar 4, 2021
3cc53af
DOC GH4094 Remove comment
Mar 4, 2021
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.2.4.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Fixed regressions
Bug fixes
~~~~~~~~~

-
- Bug in :meth:`~DataFrame.update` unnecessarily changing the dtype of a column (:issue:`4094`)
-

.. ---------------------------------------------------------------------------
Expand Down
7 changes: 6 additions & 1 deletion pandas/core/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6939,6 +6939,7 @@ def update(
if not isinstance(other, DataFrame):
other = DataFrame(other)

other_dtypes = other.dtypes # dtype might change during reindexing
other = other.reindex_like(self)

for col in self.columns:
Expand All @@ -6963,7 +6964,11 @@ def update(
if mask.all():
continue

self[col] = expressions.where(mask, this, that)
col_array = expressions.where(mask, this, that)
Copy link
Contributor

Choose a reason for hiding this comment

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

what are you trying to do here? infer and cast? this is very awkward what you are doing.
.where casts to the most appropriate type, but then you are casting back?

Copy link
Author

@Horstage Horstage Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

.reindex_like changes the dtype to a nullable dtype if other.index is a subset of self.index.
.where receives a nullable dtype and continues to use it, even though the NaNs are masked.
I catch the case where self[col] and other[col] originally have the same dtype, but .where returns a different dtype.
With the assumption, that all NaNs are masked I am casting back, yes.

Example:

self[col] = Series(["a", "b"], dtype=str)
other[col] = Series(["c"], dtype=str)
.reindex -> other[col] = Series(["c", None], dtype=object)
.where -> self[col] = Series(["c", "b"], dtype=object)

Copy link
Contributor

Choose a reason for hiding this comment

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

where is .reindex_like called? this should be fixed at the sort not patched over

Copy link

Choose a reason for hiding this comment

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

reindex_like is called here: https://github.com/pandas-dev/pandas/pull/40219/files#diff-421998af5fe0fbc54b35773ce9b6289cae3e8ae607f81783af04ebf1fbcaf077R6943 to have the same index in the other as in self, but for missing indices it inserts NANs and changes data type, maybe reindex_like needs to be changed in this case...

if self[col].dtype == other_dtypes[col] != col_array.dtype:
self[col] = Series(col_array, index=self.index, dtype=self[col].dtype)
else:
self[col] = col_array

# ----------------------------------------------------------------------
# Data reshaping
Expand Down
42 changes: 42 additions & 0 deletions pandas/tests/frame/methods/test_update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from hypothesis import given
from hypothesis.extra import numpy as np_st
import numpy as np
import pytest

Expand Down Expand Up @@ -146,3 +148,43 @@ def test_update_with_different_dtype(self):

expected = DataFrame({"a": [1, 3], "b": [np.nan, 2], "c": ["foo", np.nan]})
tm.assert_frame_equal(df, expected)

@given(dtype=np_st.integer_dtypes(endianness="="))
def test_update_with_subset_and_same_integer_dtype(self, dtype):
# GH4094
df = DataFrame({"a": Series([1, 2, 3], dtype=dtype)})
update = df.copy()[:-1]
df.update(update)
assert df.a.dtype == dtype

def test_update_str_dtype(self):
# GH4094
df = DataFrame({"a": ["a", "b", "c"]}, dtype="string")
update = df.copy()
expected = df.copy()
df.update(update)
assert df.a.dtype == expected.a.dtype

def test_update_with_subset_str_dtype(self):
# GH4094
df = DataFrame({"a": ["a", "b", "c"]}, dtype="string")
update = df.copy()[:-1]
expected = df.copy()
df.update(update)
assert df.a.dtype == expected.a.dtype

def test_update_bool_dtype(self):
# GH4094
df = DataFrame({"a": [True, False, True]}, dtype=bool)
update = df.copy()
expected = df.copy()
df.update(update)
assert df.a.dtype == expected.a.dtype

def test_update_with_subset_bool_dtype(self):
# GH4094
df = DataFrame({"a": [True, False]}, dtype=bool)
update = DataFrame({"a": [False]}, dtype=bool)
expected = df.copy()
df.update(update)
assert df.a.dtype == expected.a.dtype