support passing a function to combine_attrs

[keewis]

Allowing to pass a function to combine_attrs is important if a user wants to do something the builtin merge strategies don't support.

• towards Keep attrs by default? (keep_attrs) #3891
• Tests added
• Passes pre-commit run --all-files
• User visible changes (including notable bug fixes) are documented in whats-new.rst

[dcherian]

One thing to think about is .encoding.

IIUC we want to treat .encoding like .attrs eventually, so we need to design the signature of this callable to handle that especially since .encoding can contain "useful" attributes like "coordinates" and "bounds" after #2844.

One option would be

def my_combine_attrs(list_of_attrs_dicts, list_of_encoding_dicts):
attrs = ...
encoding = ...
return attrs, encoding

[keewis]

sounds reasonable, but that would require a bigger change than just extending merge_attrs (which currently ignores encoding)

[max-sixty]

Shall we merge?

[keewis]

not sure. There are a few questions about the signature of the user functions (see #4896 (comment) and #3891 (comment)), which I would like to answer before including this in a release (I might be wrong, but I think changing the signature after releasing is pretty hard)

[dcherian]

Thanks @keewis

[keewis]

there's two remaining issues: should we use a namedtuple / a dataclass for Context instead of the custom class? We basically want a struct without methods (for now?).

Also, how do we best construct that object without a lot of overhead? We need to get at least the function name, but if we pass that manually it's one more place to update when renaming something (not that we do that very often). Using inspect is possible but might be too complicated:

In [5]: importinspect
...: ...: defcurrent_function_name():
...: frame=inspect.currentframe()
...: try:
...: caller=frame.f_back
...: name=caller.f_code.co_name
...: finally:
...: delframe
...: delcaller
...: ...: returnname
...: ...: deffunc():
...: print(current_function_name())
...: ...: defanother_func():
...: print(current_function_name())
...: ...: classA:
...: defmethod(self):
...: print(current_function_name())
...: ...: f=func
...: ...: func()
...: another_func()
...: f()
...: ...: a=A()
...: a.method()
funcanother_funcfuncmethod

With this we can only get the name of the definition so this might break for injected methods, but I guess for injected methods it would be difficult to manually pass the function name, anyways.

[shoyer]

Also, how do we best construct that object without a lot of overhead? We need to get at least the function name, but if we pass that manually it's one more place to update when renaming something (not that we do that very often). Using inspect is possible but might be too complicated:

Rather than introspection, I think we should try to be fully explicit about the function being called. Trying to introspect it from stack-frames is madness :)

So in that case, we would need to pass down the context information from the top level functions in xarray, e.g., everything that takes a combine_attrs argument.

In terms of the overall interface, one other concern I have is about the information we make available to users of this API. I can imagine that they might not only want attributes but also the complete xarray objects on which the function is being called. If that's the case, then they would also need more information from the global context.

[keewis]

let's merge this as-is (unless there are any comments on the current state?) and I'll add the construction of the context objects in a new PR.

[dcherian]

let's merge this as-is

Works for me. Once merged perhaps @huard or @DamienIrving can help us iterate on context

[keewis]

thanks for the reviews, @shoyer, @dcherian, @max-sixty

[github-actions]

Unit Test Results

0 files ±0 0 suites ±0 0s ⏱️ ±0s
0 tests ±0 0 ✔️ ±0 0 💤 ±0 0 ❌ ±0 

Results for commit e87d65b. ± Comparison against base commit e87d65b.