Follow keep_attrs in Dataset binary ops

[arongergely]

• Closes Dataset binary ops ignore keep_attrs option #7390
• Tests added
• User visible changes (including notable bug fixes) are documented in whats-new.rst

Should I describe in whats-new.rst?

[keewis]

thanks for the PR, @arongergely, and sorry for the delayed reply.

If possible I'd like to change the computation of the attrs of the result a bit to allow more than just bool values (see below).

And yes, a entry to whats-new.rst would be great (section "Bug fixes").

[arongergely]

Thanks for your suggestions @keewis!

If possible I'd like to change the computation of the attrs of the result a bit to allow more than just bool values (see below).

I was puzzled initially. We would introduce the combine_attrs behaviour of xarray.merge() but under keep_attrs.
Then I found out this is already a thing in apply_ufunc().

I like the idea, shouldn't we

• implement this for xarray.Variable too?
• consider this as new feature, not just a bugfix? (for describing in whats-new.rst)
• describe in the documentation? (also for apply_ufunc() - it's not yet described there).

could do all these, let me know

[keewis]

The missing documentation for this in apply_ufunc is definitely an oversight, so PRs very welcome!

consider this as new feature, not just a bugfix?

I always struggle with classifying changes, but I guess the more we add onto this PR, the more it becomes a new feature.

implement this for xarray.Variable too?

probably. The only reason this is not done yet is because nobody took the time to actually do it. So, once again: PRs very welcome!

[arongergely]

[arongergely]

Hit a roadblock.

For binary ops the only way to set keep_attrs is through the options.
However we can not set combine_attrs style strings to it due to strict validation.
So this would lead to an error:

with xr.set_options(keep_attrs="drop_conflicts"):
   ds1 + ds2

To cricle around this we could let keep_attrs passed in the operator methods so user could do:

ds1.__add__(ds2, keep_attrs="drop_conflicts")

But this isn't "nice".

Or perhaps I missed something?

[arongergely]

Changing the option setters to accept combine_attrs-style strings would open room for silent bugs:
Other funcs/operators assume keep_attrs to be boolean so an if keep_attrs would cause trouble.

[keewis]

I guess this means that we have two options: just assume this does not cause anything else to fail to get this PR to work (but then this PR would be blocked until we fixed every use of the global keep_attrs in a separate PR), or we ignore str-valued / callable keep_attrs for now and merge your initial proposal.

Option 2 seems easier, so I'd suggest we go with that?

[arongergely]

Option 2 seems easier, so I'd suggest we go with that?

Agreed!

[arongergely]

Will you cherry pick?

[arongergely]

maybe not. I changed it back so you could squash to exclude the noise.
The test is adjusted accordingly.

[keewis]

Thanks, @arongergely. We always squash-merge so this is basically what I would have done.

[dcherian]

Thanks @arongergely !