-
-
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 3 commits
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 |
---|---|---|
|
@@ -3266,13 +3266,23 @@ def combine_first(self, other) -> Series: | |
falcon NaN | ||
dtype: float64 | ||
""" | ||
from pandas.core.reshape.concat import concat | ||
|
||
new_index = self.index.union(other.index) | ||
this = self.reindex(new_index, copy=False) | ||
other = other.reindex(new_index, copy=False) | ||
|
||
this = self | ||
null_mask = isna(this) | ||
if null_mask.any(): | ||
drop = this.index[null_mask].intersection(other.index[notna(other)]) | ||
if len(drop): | ||
this = this.drop(drop) | ||
|
||
other = other.reindex(other.index.difference(this.index), copy=False) | ||
if this.dtype.kind == "M" and other.dtype.kind != "M": | ||
other = to_datetime(other) | ||
|
||
return this.where(notna(this), other) | ||
combined = concat([this, other]).reindex(new_index, copy=False) | ||
combined.name = self.name | ||
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.
Can you add a quick comment what this is doing? Took me a while to figure it out, don't want to do this again in 6 months time :)
Otherwise lgtm
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.
i've simplified this logic a bit. should be easier to follow now