Skip to content

Conversation

@dcherian
Copy link
Contributor

@dcherian dcherian commented Jan 2, 2025

Closes #9898

  • Closes #xxxx
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@dcherian dcherian added the run-upstream Run upstream CI label Jan 2, 2025
@dcherian
Copy link
Contributor Author

dcherian commented Jan 2, 2025

Now that we don't fail on the warnings, there are upstream Zarr failures e.g. zarr-developers/zarr-python#2627

@dcherian dcherian merged commit 1622499 into pydata:main Jan 2, 2025
27 of 29 checks passed
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 19, 2025
* main: (63 commits)
  Fix zarr upstream tests (pydata#9927)
  Update pre-commit hooks (pydata#9925)
  split out CFDatetimeCoder, deprecate use_cftime as kwarg (pydata#9901)
  dev whats-new (pydata#9923)
  Whats-new 2025.01.0 (pydata#9919)
  Silence upstream Zarr warnings (pydata#9920)
  time coding refactor (pydata#9906)
  fix warning from scipy backend guess_can_open on directory (pydata#9911)
  Enhance and move ISO-8601 parser to coding.times (pydata#9899)
  Edit serialization error message (pydata#9916)
  friendlier error messages for missing chunk managers (pydata#9676)
  Bump codecov/codecov-action from 5.1.1 to 5.1.2 in the actions group (pydata#9915)
  Rewrite interp to use `apply_ufunc` (pydata#9881)
  Skip dask rolling (pydata#9909)
  Explicitly configure ReadTheDocs build to use conf.py (pydata#9908)
  Cache pre-existing Zarr arrays in Zarr backend (pydata#9861)
  Optimize idxmin, idxmax with dask (pydata#9800)
  remove unused "type: ignore" comments in test_plot.py (fixed in matplotlib 3.10.0) (pydata#9904)
  move scalar-handling logic into `possibly_convert_objects` (pydata#9900)
  Add missing DataTree attributes to docs (pydata#9876)
  ...
max-sixty added a commit to max-sixty/xarray that referenced this pull request Sep 9, 2025
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
max-sixty added a commit that referenced this pull request Oct 8, 2025
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-upstream Run upstream CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

⚠️ Nightly upstream-dev CI failed ⚠️

1 participant