Fix binary operations on attrs for Series and DataFrame

[fbourgey]

• closes BUG: binary operations don't propogate attrs depending on order with Series and/or DataFrame/Series #51607
• Test
• Test

[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]

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

[fbourgey]

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]

@mroeschke should everything be rewritten using finalize then?

[mroeschke]

Yes, or __finalize__ needs to be probably be called somewhere

[github-actions]

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?

[mroeschke]

It appears that _construct_result is the common denominator method that is called after these methods. I think other should be passed along there where __finalize__(other) is ultimately called

[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]

Could you type this argument?

[fbourgey]

Suggested change

	other=None,
	other: Series | None = None,

like this?

[mroeschke]

I'm guessing ArrayLike but I'm also not familiar with this code path

[mroeschke]

Shouldn't this be result.__finalize__(other) (after the if isinstance(result, tuple) is evaluated)?

[fbourgey]

Doing

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

after the block

if isinstance(result, tuple):
   ...

breaks as result is an ArrayLike

[mroeschke]

Or rather out.__finalize__(other)?

[mroeschke]

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

[fbourgey]

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.

[mroeschke]

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

[mroeschke]

I think this could be simplified to

@pytest.mark.parametrize("left", [pd.Series, pd.DataFrame])
@pytest.mark.parametrize("right", [pd.Series, pd.DataFrame])
def test_attrs_binary_operations(...):
    left = left([1])
    left.attrs = {"a": 1}
    right = right([2])
    ...

[fbourgey]

done, thanks.