Fix binary operations on attrs for Series and DataFrame

[fbourgey]

• [ ] closes #51607
• Test
• Test

[fbourgey]

@mroeschke should everything be rewritten using finalize then?

[mroeschke]

Yes, or __finalize__ needs to be probably be called somewhere

[github-actions[bot]]

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]

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

[WillAyd]

I think it looks good but will defer to @mroeschke

[WillAyd]

I don't think the CI failures are related. This still lgtm - @mroeschke can you take another look?

[fbourgey]

@WillAyd @mroeschke, any chance I could get an update?

[fbourgey]

@mroeschke I suggested something. I kept some checks for attrs in _cmp_method and _arithm_method in pandas/core/frame.py

[a-holm]

This looks like a good job. However, I noticed some conditional logic added in frame.py (_cmp_method, _arith_method, _construct_result) and series.py (_construct_result):

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

This logic seems to attempt copying attributes from the other (right) operand if self (left) doesn't initially have attributes. This appears potentially incorrect, as standard pandas behavior (and the new test) suggests the result should only inherit attributes from the left operand (self). The final out.__finalize__(self) call in _construct_result should already handle this correctly.

Could we simplify the implementation by removing these conditional self.__finalize__(other) calls and relying solely on out.__finalize__(self)? This seems like it would align better with the intended behavior and the added test case. However I might be wrong, just wanted to point this out in case something there was unintentional.

[mroeschke]

Thanks for sticking with this @fbourgey!

[fbourgey]

Thanks for the help @mroeschke!