-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
Fix binary operations on attrs for Series and DataFrame #59636
Changes from 32 commits
38efbaa
15db834
50118d7
953ef17
bc4da65
2607f9c
b154203
87abede
8559c1f
742e3b1
5178113
5b71171
8da62bd
c2c87dd
4da1786
6436926
50396ad
76b0831
ef3c0b5
e92e431
311b662
95cb745
c162bba
bd46491
bf898c7
e9c3e91
b72a049
3abc22d
1e6197c
3a80cf9
83e908f
e342a6e
c251196
96ddd0d
155a9d1
45e2ad0
cf67fcf
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 | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -7879,7 +7879,7 @@ def _cmp_method(self, other, op): | |||||||
|
||||||||
# See GH#4537 for discussion of scalar op behavior | ||||||||
new_data = self._dispatch_frame_op(other, op, axis=axis) | ||||||||
return self._construct_result(new_data) | ||||||||
return self._construct_result(new_data, other=other) | ||||||||
|
||||||||
def _arith_method(self, other, op): | ||||||||
if self._should_reindex_frame_op(other, op, 1, None, None): | ||||||||
|
@@ -7892,7 +7892,7 @@ def _arith_method(self, other, op): | |||||||
|
||||||||
with np.errstate(all="ignore"): | ||||||||
new_data = self._dispatch_frame_op(other, op, axis=axis) | ||||||||
return self._construct_result(new_data) | ||||||||
return self._construct_result(new_data, other=other) | ||||||||
|
||||||||
_logical_method = _arith_method | ||||||||
|
||||||||
|
@@ -8088,8 +8088,7 @@ def _align_for_op( | |||||||
|
||||||||
Parameters | ||||||||
---------- | ||||||||
left : DataFrame | ||||||||
right : Any | ||||||||
other : Any | ||||||||
axis : int | ||||||||
flex : bool or None, default False | ||||||||
Whether this is a flex op, in which case we reindex. | ||||||||
|
@@ -8101,6 +8100,7 @@ def _align_for_op( | |||||||
left : DataFrame | ||||||||
right : Any | ||||||||
""" | ||||||||
|
||||||||
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.
Suggested change
|
||||||||
left, right = self, other | ||||||||
|
||||||||
def to_series(right): | ||||||||
|
@@ -8200,7 +8200,6 @@ def to_series(right): | |||||||
"`left, right = left.align(right, axis=1)` " | ||||||||
"before operating." | ||||||||
) | ||||||||
|
||||||||
left, right = left.align( | ||||||||
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.
Suggested change
|
||||||||
right, | ||||||||
join="outer", | ||||||||
|
@@ -8209,6 +8208,9 @@ def to_series(right): | |||||||
) | ||||||||
right = left._maybe_align_series_as_frame(right, axis) | ||||||||
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. I think this resets the 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. I would consider that a bug. 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. Should I fix it in this PR or raise a different issue? 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. You can fix it in this PR 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. Suggested something below |
||||||||
|
||||||||
# Ensure attributes are consistent between the aligned and original objects | ||||||||
if right.attrs != getattr(other, "attrs", {}): | ||||||||
right.attrs = getattr(other, "attrs", {}).copy() | ||||||||
return left, right | ||||||||
|
||||||||
def _maybe_align_series_as_frame(self, series: Series, axis: AxisInt): | ||||||||
|
@@ -8269,9 +8271,9 @@ def _flex_arith_method( | |||||||
|
||||||||
new_data = self._dispatch_frame_op(other, op) | ||||||||
|
||||||||
return self._construct_result(new_data) | ||||||||
return self._construct_result(new_data, other=other) | ||||||||
|
||||||||
def _construct_result(self, result) -> DataFrame: | ||||||||
def _construct_result(self, result, other=None) -> DataFrame: | ||||||||
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.
Suggested change
Might as well make this required |
||||||||
""" | ||||||||
Wrap the result of an arithmetic, comparison, or logical operation. | ||||||||
|
||||||||
|
@@ -8283,6 +8285,8 @@ def _construct_result(self, result) -> DataFrame: | |||||||
------- | ||||||||
DataFrame | ||||||||
""" | ||||||||
if not getattr(self, "attrs", None) and getattr(other, "attrs", None): | ||||||||
self.__finalize__(other) | ||||||||
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. Can we do |
||||||||
out = self._constructor(result, copy=False).__finalize__(self) | ||||||||
# Pin columns instead of passing to constructor for compat with | ||||||||
# non-unique columns case | ||||||||
|
@@ -8308,7 +8312,7 @@ def _flex_cmp_method(self, other, op, *, axis: Axis = "columns", level=None): | |||||||
self, other = self._align_for_op(other, axis, flex=True, level=level) | ||||||||
|
||||||||
new_data = self._dispatch_frame_op(other, op, axis=axis) | ||||||||
return self._construct_result(new_data) | ||||||||
return self._construct_result(new_data, other=other) | ||||||||
|
||||||||
@Appender(ops.make_flex_doc("eq", "dataframe")) | ||||||||
def eq(self, other, axis: Axis = "columns", level=None) -> DataFrame: | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7148,10 +7148,10 @@ def _logical_method(self, other, op): | |||||
rvalues = extract_array(other, extract_numpy=True, extract_range=True) | ||||||
|
||||||
res_values = ops.logical_op(lvalues, rvalues, op) | ||||||
return self._construct_result(res_values, name=res_name) | ||||||
return self._construct_result(res_values, name=res_name, other=other) | ||||||
|
||||||
@final | ||||||
def _construct_result(self, result, name): | ||||||
def _construct_result(self, result, name, other=None): | ||||||
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.
Suggested change
|
||||||
if isinstance(result, tuple): | ||||||
return ( | ||||||
Index(result[0], name=name, dtype=result[0].dtype), | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5858,7 +5858,7 @@ def _cmp_method(self, other, op): | |||||
|
||||||
res_values = ops.comparison_op(lvalues, rvalues, op) | ||||||
|
||||||
return self._construct_result(res_values, name=res_name) | ||||||
return self._construct_result(res_values, name=res_name, other=other) | ||||||
|
||||||
def _logical_method(self, other, op): | ||||||
res_name = ops.get_op_result_name(self, other) | ||||||
|
@@ -5868,7 +5868,7 @@ def _logical_method(self, other, op): | |||||
rvalues = extract_array(other, extract_numpy=True, extract_range=True) | ||||||
|
||||||
res_values = ops.logical_op(lvalues, rvalues, op) | ||||||
return self._construct_result(res_values, name=res_name) | ||||||
return self._construct_result(res_values, name=res_name, other=other) | ||||||
|
||||||
def _arith_method(self, other, op): | ||||||
self, other = self._align_for_op(other) | ||||||
|
@@ -5930,11 +5930,15 @@ def _binop(self, other: Series, func, level=None, fill_value=None) -> Series: | |||||
result = func(this_vals, other_vals) | ||||||
|
||||||
name = ops.get_op_result_name(self, other) | ||||||
out = this._construct_result(result, name) | ||||||
|
||||||
out = this._construct_result(result, name, other) | ||||||
return cast(Series, out) | ||||||
|
||||||
def _construct_result( | ||||||
self, result: ArrayLike | tuple[ArrayLike, ArrayLike], name: Hashable | ||||||
self, | ||||||
result: ArrayLike | tuple[ArrayLike, ArrayLike], | ||||||
name: Hashable, | ||||||
other: AnyArrayLike | DataFrame | None = None, | ||||||
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.
Suggested change
|
||||||
) -> Series | tuple[Series, Series]: | ||||||
""" | ||||||
Construct an appropriately-labelled Series from the result of an op. | ||||||
|
@@ -5943,6 +5947,7 @@ def _construct_result( | |||||
---------- | ||||||
result : ndarray or ExtensionArray | ||||||
name : Label | ||||||
other : Series, DataFrame or array-like, default None | ||||||
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.
Suggested change
|
||||||
|
||||||
Returns | ||||||
------- | ||||||
|
@@ -5952,8 +5957,8 @@ def _construct_result( | |||||
if isinstance(result, tuple): | ||||||
# produced by divmod or rdivmod | ||||||
|
||||||
res1 = self._construct_result(result[0], name=name) | ||||||
res2 = self._construct_result(result[1], name=name) | ||||||
res1 = self._construct_result(result[0], name=name, other=other) | ||||||
res2 = self._construct_result(result[1], name=name, other=other) | ||||||
|
||||||
# GH#33427 assertions to keep mypy happy | ||||||
assert isinstance(res1, Series) | ||||||
|
@@ -5969,6 +5974,8 @@ def _construct_result( | |||||
# Set the result's name after __finalize__ is called because __finalize__ | ||||||
# would set it back to self.name | ||||||
out.name = name | ||||||
if not getattr(self, "attrs", None) and getattr(other, "attrs", None): | ||||||
fbourgey marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
out.__finalize__(other) | ||||||
fbourgey marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
return out | ||||||
|
||||||
def _flex_method(self, other, op, *, level=None, fill_value=None, axis: Axis = 0): | ||||||
|
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.