-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix assert_equal with check_dim_order=False for mixed dimension orders #10718
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
base: main
Are you sure you want to change the base?
Conversation
…imension orders Fixes pydata#10704 The bug: assert_equal with check_dim_order=False was failing when comparing Datasets containing variables with different dimension orders. It would even fail when comparing a Dataset to itself. The fix: Transpose both objects to a canonical dimension order using the intersection of their dimensions. The ellipsis (...) handles any dimensions unique to either object, making the solution general and elegant. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
xarray/testing/assertions.py
Outdated
# DataTree case needs special handling - only transpose b | ||
return a, map_over_datasets(lambda a, b: _maybe_transpose_dims(a, b)[1], a, b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks, sorry for outsourcing finding this. we should find a better balance of using AI agents; I do worry about this sort of OSS interaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example — if an AI agent writes the code, it needs to have 100% test coverage before a human is asked to review it
- Use list(common_dims) instead of sorted(common_dims) since dimensions only need to be hashable, not sortable - Add test case for datasets with non-sortable dimension names (e.g., int and str) - Transpose both a and b to the same canonical order for consistency 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
for more information, see https://pre-commit.ci
- Add test for no common dimensions path - Add test for Variable type specifically (not just DataArray) - Now all code paths in maybe_transpose_dims are covered 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
for more information, see https://pre-commit.ci
The reviewer was correct - the DataTree handling was wrong. We were only transposing b, but for consistency we need to transpose both a and b to the same canonical order, just like we do for Dataset. This fixes the issue and adds a comprehensive test case. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
for more information, see https://pre-commit.ci
Summary
assert_equal
withcheck_dim_order=False
now works correctly for Datasets containing variables with different dimension ordersThe Bug
When comparing Datasets with
check_dim_order=False
, the comparison would fail if individual variables had different dimension orders, even when comparing a Dataset to itself:The Fix
The solution transposes both objects to a canonical dimension order using the intersection of their dimensions. The ellipsis (
...
) handles any dimensions unique to either object:This elegant approach works uniformly for Variables, DataArrays, and Datasets without special casing.
Test plan
test_assert_equal_dataset_check_dim_order
that reproduces the issueassert_equal
andassert_allclose
🤖 Generated with Claude Code