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.

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

rolling keep_attrs & default True #4510

Merged
merged 20 commits into from
Nov 9, 2020
Merged

Conversation

mathause
Copy link
Collaborator

Only global attributes were retained for rolling operations. DataArrays would loose their attrs.

As per #3891 (comment) I also changed the default to True.

Note #4497 also mentions propagation of the name. This is not yet implemented.

Comment on lines 815 to 816
If True (default), the object's attributes (`attrs`) will be copied
from the original object to the new one. If False, the new
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
If True (default), the object's attributes (`attrs`) will be copied
from the original object to the new one. If False, the new
If None (default) or True, the object's attributes (`attrs`) will be
copied from the original object to the new one. If False, the new

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd replace the (default) and the , optional in the type spec with , default: None

@max-sixty
Copy link
Collaborator

Great, thanks!

Are the global configs still being honored in rolling? No strong view on whether they should be, though it affects whether we have defaults of None.

xarray/core/rolling.py Outdated Show resolved Hide resolved
@mathause
Copy link
Collaborator Author

As mentioned in #4513 I should probably switch from ds.rolling(..., keep_attrs=True).mean() to ds.rolling(...).mean( keep_attrs=True). Let's see if there is feedback/ a consensus on #4513

doc/whats-new.rst Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@mathause mathause left a comment

Choose a reason for hiding this comment

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

This is ready for a review. It was a bit more complicated than anticipated - I had to thread keep_attrs through a lot of functions. So may not be trivial to check.

To summarize:

xarray/core/rolling.py Outdated Show resolved Hide resolved
Comment on lines +477 to +479
f"Reductions are applied along the rolling dimension(s) "
f"'{self.dim}'. Passing the 'dim' kwarg to reduction "
f"operations has no effect.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The warning used to be:

"and will raise an error in xarray 0.16.0"

I decided to continue warning and not raise an error - objections?

@max-sixty
Copy link
Collaborator

Wow, this was quite the herculean effort @mathause . Thanks. That solves any questions I had about whether the global configs are being honored...

To the extent we have to do more of these, we could consider whether we need to add keep_attrs kwargs, or we should recommend users removing attrs separately where needed. We'd still need to through the global defaults, though.

xarray/core/common.py Outdated Show resolved Hide resolved
@max-sixty
Copy link
Collaborator

Any other thoughts before we merge?

@mathause
Copy link
Collaborator Author

I added a test to ensure da.name is conserved & the tests passed. So this is finished from my side. I guess this is not so easy to review as I change one or two lines per function in a lot of functions. So maybe someone could just review the tests? Or maybe you already did?

While working on this I thought rolling could profit from some refactoring but I kept it out of this PR. I might follow up with do this if I find time.

Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

I think this looks great. I read through the tests carefully and browed the code. I would merge unless others have thoughts

xarray/tests/test_dataarray.py Show resolved Hide resolved
Copy link
Collaborator Author

@mathause mathause left a comment

Choose a reason for hiding this comment

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

Testing a global default and kwarg revealed some more issues. Should be fixed now.

I realize this does still not preserve the encoding. Not sure what our general take is on that.

xarray/tests/test_dataarray.py Show resolved Hide resolved
)
else:
dataset[key] = da
return Dataset(dataset, coords=self.obj.coords).isel(
dataset[key] = da.copy()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I now do a deep copy here. I think this is safer. Else non-rolled DataArrays were connected to the original obj.

@mathause mathause added the topic-metadata Relating to the handling of metadata (i.e. attrs and encoding) label Oct 30, 2020
xarray/core/common.py Outdated Show resolved Hide resolved
xarray/core/common.py Outdated Show resolved Hide resolved
xarray/core/common.py Outdated Show resolved Hide resolved
@mathause mathause merged commit 1735892 into pydata:master Nov 9, 2020
@mathause mathause deleted the rolling_keep_attrs branch November 9, 2020 15:35
@max-sixty
Copy link
Collaborator

Thanks again @mathause !

@keewis keewis mentioned this pull request Feb 17, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-metadata Relating to the handling of metadata (i.e. attrs and encoding)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ds.rolling() drops attributes and name
3 participants