-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG/PERF: Series.combine_first converting int64 to float64 #51777
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.
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
Changes from 1 commit
d28c39c
f60a3d6
e10b62b
d471b61
5ea12f3
b2e9a88
eb51e4d
63fdc25
3e60a36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,11 +93,13 @@ | |
from pandas.core.dtypes.cast import ( | ||
LossySetitemError, | ||
convert_dtypes, | ||
find_common_type, | ||
maybe_box_native, | ||
maybe_cast_pointwise_result, | ||
) | ||
from pandas.core.dtypes.common import ( | ||
is_dict_like, | ||
is_dtype_equal, | ||
is_extension_array_dtype, | ||
is_integer, | ||
is_iterator, | ||
|
@@ -3272,7 +3274,13 @@ def combine_first(self, other) -> Series: | |
if this.dtype.kind == "M" and other.dtype.kind != "M": | ||
other = to_datetime(other) | ||
|
||
return this.where(notna(this), other) | ||
combined = this.where(notna(this), other) | ||
|
||
if not is_dtype_equal(combined.dtype, self.dtype): | ||
dtype = find_common_type([self.dtype, other.dtype]) | ||
combined = combined.astype(dtype, copy=False) | ||
|
||
return combined | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a finalise call with self, otherwise we will loose metadata. This might changed result ordering? Can we do a reindex in the end? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated, thanks |
||
|
||
def update(self, other: Series | Sequence | Mapping) -> None: | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this won't break anything, but for the series case there might be a proper fix. You could set a compatible fill_value for the reindex ops. Only disadvantage is, that you'll have to compute
notna(this)
somehow