Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

create the context objects passed to custom combine_attrs functions #5668

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

create the context objects passed to custom combine_attrs functions #5668

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
14ed862
add a custom `repr` to the context class
keewis Aug 3, 2021
7d3e333
allow to pass a function to the `keep_attrs` option
keewis Aug 3, 2021
8fedd84
add the context object to the reduce methods
keewis Aug 3, 2021
8d23032
use `.attrs` to avoid passing `None`
keewis Aug 3, 2021
ccbcfd2
Merge branch 'main' into attrs_context
keewis Jun 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions xarray/core/merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ class Context:
def __init__(self, func):
self.func = func
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keewis do you have an idea of what to do for groupby.mean and similar calls?

Copy link
Collaborator Author

@keewis keewis Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


def __repr__(self):
return f"Context('{self.func}')"


def broadcast_dimension_size(variables: List[Variable]) -> Dict[Hashable, int]:
"""Extract dimension sizes from a dictionary of variables.
Expand Down
8 changes: 5 additions & 3 deletions xarray/core/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def _positive_integer(value):
DISPLAY_EXPAND_DATA: lambda choice: choice in [True, False, "default"],
ENABLE_CFTIMEINDEX: lambda value: isinstance(value, bool),
FILE_CACHE_MAXSIZE: _positive_integer,
KEEP_ATTRS: lambda choice: choice in [True, False, "default"],
KEEP_ATTRS: lambda choice: choice in [True, False, "default"] or callable(choice),
WARN_FOR_UNCLOSED_FILES: lambda value: isinstance(value, bool),
}

Expand Down Expand Up @@ -82,11 +82,13 @@ def _get_boolean_with_default(option, default):

if global_choice == "default":
return default
elif global_choice in [True, False]:
elif global_choice in [True, False] or callable(global_choice):
return global_choice
elif callable(global_choice):
return global_choice
else:
raise ValueError(
f"The global option {option} must be one of True, False or 'default'."
f"The global option {option} must be one of True, False or 'default' or a callable."
)


Expand Down
10 changes: 9 additions & 1 deletion xarray/core/variable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1740,6 +1740,8 @@ def reduce(
Array with summarized data and the indicated dimension(s)
removed.
"""
from .merge import Context, merge_attrs

if dim == ...:
dim = None
if dim is not None and axis is not None:
Expand Down Expand Up @@ -1782,7 +1784,13 @@ def reduce(

if keep_attrs is None:
keep_attrs = _get_keep_attrs(default=False)
attrs = self._attrs if keep_attrs else None

if isinstance(keep_attrs, bool):
keep_attrs = "override" if keep_attrs else "drop"
Comment on lines +1866 to +1867
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this go in _get_keep_attrs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, because we need to support both of these:

Suggested change
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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


attrs = merge_attrs(
[self.attrs], combine_attrs=keep_attrs, context=Context(func.__name__)
)

return Variable(dims, data, attrs=attrs)

Expand Down