create the context objects passed to custom combine_attrs functions - #5668
#5668create the context objects passed to custom combine_attrs functions#5668keewis wants to merge 5 commits into
combine_attrs functions#5668Conversation
Unit Test Results 6 files 6 suites 51m 18s ⏱️ Results for commit 8d23032. ♻️ This comment has been updated with latest results. |
| if isinstance(keep_attrs, bool): | ||
| keep_attrs = "override" if keep_attrs else "drop" |
There was a problem hiding this comment.
can this go in _get_keep_attrs?
There was a problem hiding this comment.
no, because we need to support both of these:
| if isinstance(keep_attrs, bool): | |
| keep_attrs = "override" if keep_attrs else "drop" | |
| with xr.set_options(keep_attrs=True): | |
| ds.mean() | |
| ds.mean(keep_attrs=True) |
but I can move it to merge_attrs
There was a problem hiding this comment.
merge_attrs would be a good solution.
We could update _get_keep_attrs to _get_keep_attrs(keep_attrs, default=False) and put all the logic in one place but that's a much bigger change.
| @@ -63,6 +63,9 @@ class Context: | ||
| def __init__(self, func): | ||
| self.func = func |
There was a problem hiding this comment.
@keewis do you have an idea of what to do for groupby.mean and similar calls?
There was a problem hiding this comment.
it should be possible to pass the actual (bound?) function (e.g. ds.groupby(...).mean) instead, which would also allow accessing additional data in the combine_attrs function. Not sure how to get a nice repr for that but I guess that's a minor issue.
Edit: to avoid introspecting stack frames, I guess we could pass getattr(self, "name") (where that makes sense)
Follow-up to #4896: this creates the context object in reduce methods and passes it to
merge_attrs, with more planned.pre-commit run --all-fileswhats-new.rstapi.rstNote that for now this is a bit inconvenient to use for provenance tracking (as discussed in the
cf-xarrayissue) because functions implementing that would still have to deal with merging theattrs.