Fix binary operations on attrs for Series and DataFrame by fbourgey · Pull Request #59636 · pandas-dev/pandas

fbourgey · 2024-08-28T00:10:49Z

WillAyd

Small change to prefer fixtures to writing out our own binop implementations, but generally lgtm. I don't think current CI failures are related.

@mroeschke any thoughts here?

WillAyd · 2024-09-20T18:19:23Z

+        df_2 = DataFrame({"A": [-3, 9]})
+        attrs = {"info": "DataFrame"}
+        df_1.attrs = attrs
+        assert (df_1 + df_2).attrs == attrs


Rather than doing this you can just use the all_binary_operators fixture from conftest.py (I think)

I made the change.

mroeschke

I think attrs propagation logic should should only be handled by __finalize__, so these binary operations should dispatch to that method

fbourgey · 2024-10-16T12:06:28Z

@mroeschke should everything be rewritten using finalize then?

mroeschke · 2024-10-29T20:13:12Z

Yes, or __finalize__ needs to be probably be called somewhere

github-actions · 2024-11-29T00:07:44Z

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

fbourgey · 2024-12-02T22:23:34Z

@mroeschke, @WillAyd, I tried using __finalize__ instead. What do you think?

WillAyd · 2024-12-03T16:45:01Z

I think it looks good but will defer to @mroeschke

…ns. Moved from base.py to series.py

mroeschke · 2025-03-31T16:41:23Z

    def _cmp_method(self, other, op):
        axis: Literal[1] = 1  # only relevant for Series other case

+        if not getattr(self, "attrs", None) and getattr(other, "attrs", None):


I don't think we should need these anymore here since this should be handled in _construct_result

It seems that sometimes

self, other = self._align_for_op(other, axis, flex=False, level=None)

resets other.attrs to {}.

This is why I kept it.

Is it because other is getting overridden here? Otherwise, _align_for_op should also preserve the attrs of other.

… right inputs

…nd finalize behavior

fbourgey · 2025-04-02T12:12:01Z

@@ -8212,6 +8208,9 @@ def to_series(right):
            )
            right = left._maybe_align_series_as_frame(right, axis)


I think this resets the attrs of right

I would consider that a bug. attrs should be preserved in this function.

Should I fix it in this PR or raise a different issue?

You can fix it in this PR

Suggested something below

mroeschke · 2025-04-02T16:16:08Z

        DataFrame
        """
+        if not getattr(self, "attrs", None) and getattr(other, "attrs", None):
+            self.__finalize__(other)


Can we do out = out.__finalize(other) instead?

…on logic

mroeschke · 2025-04-02T20:53:17Z

+        return self._construct_result(new_data, other=other)

-    def _construct_result(self, result) -> DataFrame:
+    def _construct_result(self, result, other=None) -> DataFrame:


Suggested change

def _construct_result(self, result, other=None) -> DataFrame:

def _construct_result(self, result, other) -> DataFrame:

Might as well make this required

mroeschke · 2025-04-02T20:54:25Z

+        if not getattr(self, "attrs", None) and getattr(other, "attrs", None):
+            out = out.__finalize__(other)


Suggested change

if not getattr(self, "attrs", None) and getattr(other, "attrs", None):

out = out.__finalize__(other)

out = out.__finalize__(other)

Appears __finalize__ will correctly skip if other has a populated attrs

Doing this breaks the following test:

FAILED pandas/tests/generic/test_duplicate_labels.py::TestPreserves::test_binops[other1-True-add] - AssertionError FAILED pandas/tests/generic/test_duplicate_labels.py::TestPreserves::test_binops[other1-True-sub] - AssertionError

I think this line __finalize__ needs a fix:

self.flags.allows_duplicate_labels = other.flags.allows_duplicate_labels

Prioritizing False if self.flags.allows_duplicate_labels or other.flags.allows_duplicate_labels is False

What about doing in __finalize__

if isinstance(other, NDFrame): if other.attrs: # We want attrs propagation to have minimal performance # impact if attrs are not used; i.e. attrs is an empty dict. # One could make the deepcopy unconditionally, but a deepcopy # of an empty dict is 50x more expensive than the empty check. self.attrs = deepcopy(other.attrs) self.flags.allows_duplicate_labels = ( self.flags.allows_duplicate_labels and other.flags.allows_duplicate_labels )

Yup that's the correct location to fix this

mroeschke · 2025-04-02T20:54:39Z


    @final
-    def _construct_result(self, result, name):
+    def _construct_result(self, result, name, other=None):


Suggested change

def _construct_result(self, result, name, other=None):

def _construct_result(self, result, name, other):

mroeschke · 2025-04-02T20:54:55Z

+        self,
+        result: ArrayLike | tuple[ArrayLike, ArrayLike],
+        name: Hashable,
+        other: AnyArrayLike | DataFrame | None = None,


Suggested change

other: AnyArrayLike | DataFrame | None = None,

other: AnyArrayLike | DataFrame,

mroeschke · 2025-04-02T20:55:04Z

        ----------
        result : ndarray or ExtensionArray
        name : Label
+        other : Series, DataFrame or array-like, default None


Suggested change

other : Series, DataFrame or array-like, default None

other : Series, DataFrame or array-like

mroeschke · 2025-04-02T20:55:24Z

+        if getattr(other, "attrs", None):
+            out.__finalize__(other)


Suggested change

if getattr(other, "attrs", None):

out.__finalize__(other)

out = out.__finalize__(other)

Doing this breaks:

FAILED pandas/tests/generic/test_duplicate_labels.py::TestPreserves::test_binops[other1-False-add] - AssertionError FAILED pandas/tests/generic/test_duplicate_labels.py::TestPreserves::test_binops[other1-False-sub] - AssertionError

something to do with flags.allows_duplicate_labels

mroeschke · 2025-04-02T20:56:18Z

+        return self._construct_result(result, name=res_name, other=other)

-    def _construct_result(self, result, name):
+    def _construct_result(self, result, name, other=None):


Suggested change

def _construct_result(self, result, name, other=None):

def _construct_result(self, result, name, other):

mroeschke · 2025-04-02T20:56:33Z

        left : DataFrame
        right : Any
        """
+


Suggested change

mroeschke · 2025-04-02T20:56:50Z

                        "before operating."
                    )
-
            left, right = left.align(


Suggested change

left, right = left.align(

left, right = left.align(

…r.flags.allows_duplicate_labels is False

mroeschke · 2025-04-03T19:00:00Z

Thanks for sticking with this @fbourgey!

fbourgey · 2025-04-03T19:19:14Z

Thanks for the help @mroeschke!

fbourgey added 9 commits August 27, 2024 19:42

Fix binary operators on attrs for Series and DataFrame

38efbaa

Add tests for Series and DataFrame for attrs binary operations

15db834

Add getattr as other might not possess attrs as attribute

50118d7

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

953ef17

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

bc4da65

Fix some tests in pandas/tests/generic/test_finalize.py::test_binops

2607f9c

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

b154203

remove xfail tests related to attrs

87abede

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

8559c1f

WillAyd requested changes Sep 20, 2024

View reviewed changes

fbourgey added 2 commits September 24, 2024 09:51

Add all_binary_operators

742e3b1

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

5178113

mroeschke requested changes Sep 25, 2024

View reviewed changes

mroeschke added metadata _metadata, .attrs Numeric Operations Arithmetic, Comparison, and Logical operations labels Sep 25, 2024

github-actions Bot added the Stale label Nov 29, 2024

fbourgey added 5 commits December 1, 2024 14:59

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

5b71171

Merge branch 'pandas-dev:main' into series-sum-attrs

8da62bd

use __finalize__ for attrs bin ops in base.py

c2c87dd

use __finalize__ for attrs bin ops in frame.py

4da1786

use __finalize__ for attrs bin ops in series.py

6436926

WillAyd removed the Stale label Dec 3, 2024

fbourgey added 3 commits January 29, 2025 09:28

Merge branch 'main' into series-sum-attrs

50396ad

Merge branch 'pandas-dev:main' into series-sum-attrs

76b0831

ENH: Ensure attrs are copied from other in Series arithmetic operatio…

ef3c0b5

…ns. Moved from base.py to series.py

mroeschke reviewed Mar 31, 2025

View reviewed changes

Comment thread pandas/tests/generic/test_finalize.py Outdated

fbourgey added 3 commits April 1, 2025 17:37

REF: Simplify test_attrs_binary_operations by parameterizing left and…

1e6197c

… right inputs

Refactor DataFrame and Series methods to improve attribute handling a…

3a80cf9

…nd finalize behavior

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

83e908f

fbourgey commented Apr 2, 2025

View reviewed changes

Comment thread pandas/core/series.py Outdated

Refactor DataFrame alignment logic to improve attribute consistency

e342a6e

fbourgey commented Apr 2, 2025

View reviewed changes

mroeschke reviewed Apr 2, 2025

View reviewed changes

Comment thread pandas/core/series.py Outdated

fbourgey added 2 commits April 2, 2025 13:59

Refactor DataFrame and Series methods to enhance attribute finalizati…

c251196

…on logic

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

96ddd0d

mroeschke reviewed Apr 2, 2025

View reviewed changes

Comment thread pandas/core/frame.py Outdated

left : DataFrame

right : Any

"""

Copy link
Copy Markdown

Member

mroeschke Apr 2, 2025

Choose a reason for hiding this comment

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

Suggested change

mroeschke reviewed Apr 2, 2025

View reviewed changes

fbourgey added 3 commits April 3, 2025 13:57

Clean and remove unnecessry checks

155a9d1

Fix: Prioritizing False if self.flags.allows_duplicate_labels or othe…

45e2ad0

…r.flags.allows_duplicate_labels is False

Merge remote-tracking branch 'upstream/main' into series-sum-attrs

cf67fcf

mroeschke approved these changes Apr 3, 2025

View reviewed changes

mroeschke added this to the 3.0 milestone Apr 3, 2025

mroeschke merged commit 04356be into pandas-dev:main Apr 3, 2025

fbourgey deleted the series-sum-attrs branch April 3, 2025 19:17

		@@ -8212,6 +8208,9 @@ def to_series(right):
		)
		right = left._maybe_align_series_as_frame(right, axis)

	def _construct_result(self, result, other=None) -> DataFrame:
	def _construct_result(self, result, other) -> DataFrame:

		if not getattr(self, "attrs", None) and getattr(other, "attrs", None):
		out = out.__finalize__(other)

	def _construct_result(self, result, name, other=None):
	def _construct_result(self, result, name, other):

	other: AnyArrayLike \| DataFrame \| None = None,
	other: AnyArrayLike \| DataFrame,

	other : Series, DataFrame or array-like, default None
	other : Series, DataFrame or array-like

	if getattr(other, "attrs", None):
	out.__finalize__(other)
	out = out.__finalize__(other)

Uh oh!

Conversation

fbourgey commented Aug 28, 2024

Uh oh!

WillAyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mroeschke left a comment

Choose a reason for hiding this comment

Uh oh!

fbourgey commented Oct 16, 2024

Uh oh!

mroeschke commented Oct 29, 2024

Uh oh!

github-actions Bot commented Nov 29, 2024

Uh oh!

fbourgey commented Dec 2, 2024

Uh oh!

WillAyd commented Dec 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fbourgey Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourgey Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mroeschke commented Apr 3, 2025

Uh oh!

fbourgey commented Apr 3, 2025

fbourgey Apr 2, 2025 •

edited

Loading

fbourgey Apr 3, 2025 •

edited

Loading