rolling keep_attrs & default True #4510
Conversation
xarray/core/common.py
Outdated
| If True (default), the object's attributes (`attrs`) will be copied | ||
| from the original object to the new one. If False, the new |
There was a problem hiding this comment.
| 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 |
There was a problem hiding this comment.
I'd replace the (default) and the , optional in the type spec with , default: None
|
Great, thanks! Are the global configs still being honored in |
mathause
left a comment
mathause
left a comment
There was a problem hiding this comment.
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:
| f"Reductions are applied along the rolling dimension(s) " | ||
| f"'{self.dim}'. Passing the 'dim' kwarg to reduction " | ||
| f"operations has no effect.", |
There was a problem hiding this comment.
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?
|
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 |
|
Any other thoughts before we merge? |
|
I added a test to ensure While working on this I thought |
max-sixty
left a comment
max-sixty
left a comment
There was a problem hiding this comment.
I think this looks great. I read through the tests carefully and browed the code. I would merge unless others have thoughts
mathause
left a comment
mathause
left a comment
There was a problem hiding this comment.
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.
| else: | ||
| dataset[key] = da | ||
| return Dataset(dataset, coords=self.obj.coords).isel( | ||
| dataset[key] = da.copy() |
There was a problem hiding this comment.
I now do a deep copy here. I think this is safer. Else non-rolled DataArrays were connected to the original obj.
mathause
added
the
topic-metadata
|
Thanks again @mathause ! |
BREAKING CHANGE: Change keep_attrs default from False to True This changes the default behavior of xarray operations to preserve attributes by default, which better aligns with user expectations and scientific workflows where metadata preservation is critical. Migration guide: - To restore previous behavior globally: xr.set_options(keep_attrs=False) - To restore for specific operations: use keep_attrs=False parameter - Alternative: use .drop_attrs() method after operations Closes pydata#3891, pydata#4510, pydata#9920
* feat: Preserve attributes by default in all operations BREAKING CHANGE: Change keep_attrs default from False to True This changes the default behavior of xarray operations to preserve attributes by default, which better aligns with user expectations and scientific workflows where metadata preservation is critical. Migration guide: - To restore previous behavior globally: xr.set_options(keep_attrs=False) - To restore for specific operations: use keep_attrs=False parameter - Alternative: use .drop_attrs() method after operations Closes #3891, #4510, #9920 * Fix Dataset.map to properly handle coordinate attrs when keep_attrs=False The merge incorrectly preserved coordinate attributes even when keep_attrs=False. Now coordinates have their attrs cleared when keep_attrs=False, consistent with data variables. * Optimize Dataset.map coordinate attribute handling - When keep_attrs=True: restore attrs from original coords (func may have dropped them) - When keep_attrs=False: clear all attrs - More efficient than previous implementation * Simplify Dataset.map attribute handling code Group attribute operations by keep_attrs value for cleaner, more readable code with identical functionality. * Remove temporal 'now' references from comments Per Stefan's review, remove 'now' from comments that describe behavior changes, as these become stale over time. Replace with timeless descriptions that simply state the current behavior. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address remaining review comments from Stefan - Remove PR-specific comment from variable.py that only makes sense in context - Remove redundant comment from test_computation.py - Clarify comment in test_dataarray.py about argmin preserving attrs * Use drop_conflicts for binary operations attribute handling Instead of only preserving the left operand's attributes, binary operations now combine attributes from both operands using the drop_conflicts strategy: - Matching attributes (same key, same value) are kept - Conflicting attributes (same key, different values) are dropped - Non-conflicting attributes from both operands are preserved This provides more intuitive behavior when combining data with partially overlapping metadata. * Fix binary ops attrs: only merge when both operands have attrs For backward compatibility, when one operand has no attributes (None), keep the left operand's attributes instead of merging. This maintains the existing test expectations while still providing drop_conflicts behavior when both operands have attributes. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix binary ops attrs handling when operands have no attrs When one operand has no attributes (either None or empty dict), the result should have no attributes. This maintains backward compatibility and fixes test_1d_math failures. The issue was compounded by a side effect in the attrs property getter that mutates _attrs from None to {} when accessed. * Simplify binary ops attrs handling Use attrs property directly instead of checking _attrs, since the property normalizes None to {}. This simplifies the logic while maintaining the same behavior. * Clarify comments about attrs handling differences Variable uses None for no attrs, Dataset uses {} for no attrs. Updated comments to make this distinction clear. * Implement true drop_conflicts behavior for binary operations Previously we were dropping all attrs if either operand had no attrs. Now we properly merge attrs and only drop conflicting ones, which is what drop_conflicts should do: - If one has {"a": 1} and other has {}, result is {"a": 1} - If one has {"a": 1} and other has {"a": 2}, result is {} - If one has {"a": 1} and other has {"b": 2}, result is {"a": 1, "b": 2} Updated tests to reflect this correct behavior. * Remove unnecessary conversion of {} to None Variable constructor already normalizes empty dict to None internally, so the explicit conversion is redundant. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
3 participants
isort . && black . && mypy . && flake8whats-new.rstOnly global attributes were retained for
rollingoperations.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.