Add argument check_dims to assert_allclose to allow transposed inputs (#5733) #8991

ignamv · 2024-05-01T12:05:40Z

Closes Shoudn't assert_allclose transpose datasets? #5733
Tests added
User visible changes (including notable bug fixes) are documented in whats-new.rst
New functions/methods are listed in api.rst

welcome · 2024-05-01T12:05:43Z

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

TomNicholas

Thanks for working on this @ignamv !

doc/whats-new.rst

xarray/testing/assertions.py

TomNicholas · 2024-05-01T15:21:23Z

xarray/testing/assertions.py

+    if not isinstance(a, (Variable, DataArray, Dataset)):
+        return b
+    if check_dims == "transpose":
+        assert set(a.dims) == set(b.dims), f"Dimensions differ: {a.dims} {b.dims}"


Assuming that Dataset.dims returns an ordered type may no longer be true after #8980, but that's my problem, not yours 😅

This future change is also raising a warning during the tests, not sure if I should be suppressing it.

Okay so we should deal with it now then. Basically Dataset objects don't really have ordered dimensions, and Dataset.dims will be changed to return a set type in future. So check_dim_order can't do anything when comparing dataset objects, the most you can do is check that all the same dims are present.

TomNicholas · 2024-05-01T15:22:46Z

xarray/testing/assertions.py

+    if check_dims == "transpose":
+        assert set(a.dims) == set(b.dims), f"Dimensions differ: {a.dims} {b.dims}"
+        return b.transpose(*a.dims)
+    assert a.dims == b.dims, f"Dimensions differ: {a.dims} {b.dims}"


This is good - if the dimensions differ there is no point checking anything else.

Are you sure we don't have better diffing functions further down the stack? We should ensure the output of an error is no worse with this than with this commented out

The usual diff would allow you to see if two datasets somehow got created with the same values but swapped dimensions. If we want this, I can skip the transpose when check_dim_order=False and the dims differ (so it doesn't fail on transpose) and just let it make the whole diff. And I'll always skip the dims check when check_dim_order=True.

Yes I would probably let it fall through, so we have a single place where we check & generate diffs; at least unless there's a good reason not to

(though @TomNicholas let us know if you disagree!)

Co-authored-by: Tom Nicholas <tom@cworthy.org>

xarray/testing/assertions.py

max-sixty · 2024-05-01T17:11:03Z

xarray/tests/test_assertions.py

+@pytest.mark.parametrize(
+    "func", ["assert_equal", "assert_allclose", "assert_identical"]
+)
+def test_assert_allclose_equal_identical_transpose(func) -> None:


Nice test the tests!

Co-authored-by: Tom Nicholas <tom@cworthy.org>

…ims to maybe_transpose_dims

max-sixty · 2024-05-04T21:41:26Z

xarray/testing/assertions.py

@@ -162,7 +176,7 @@ def assert_identical(a: DataTree, b: DataTree, from_root: bool = True): ...


 @ensure_warnings
-def assert_identical(a, b, from_root=True):
+def assert_identical(a, b, from_root=True, check_dim_order: bool = True):


I don't think it makes sense to have this arg — identical means identical :) — but happy to defer if someone does...

xarray/testing/assertions.py

…nspose_dimensions

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

max-sixty · 2024-05-05T18:06:54Z

Thanks a lot @ignamv !

welcome · 2024-05-05T18:06:59Z

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again!

…#5733) (#8991) * Add argument check_dims to assert_allclose to allow transposed inputs * Update whats-new.rst * Add `check_dims` argument to assert_equal and assert_identical + tests * Assert that dimensions match before transposing or comparing values * Add docstring for check_dims to assert_equal and assert_identical * Update doc/whats-new.rst Co-authored-by: Tom Nicholas <tom@cworthy.org> * Undo fat finger Co-authored-by: Tom Nicholas <tom@cworthy.org> * Add attribution to whats-new.rst * Replace check_dims with bool argument check_dim_order, rename align_dims to maybe_transpose_dims * Remove left-over half-made test * Remove check_dim_order argument from assert_identical * assert_allclose/equal: emit full diff if dimensions don't match * Rename check_dim_order test, test Dataset with different dim orders * Update whats-new.rst * Hide maybe_transpose_dims from Pytest traceback Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> * Ignore mypy error due to missing functools.partial.__name__ --------- Co-authored-by: Tom Nicholas <tom@cworthy.org> Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

* main: Avoid auto creation of indexes in concat (#8872) Fix benchmark CI (#9013) Avoid extra read from disk when creating Pandas Index. (#8893) Add a benchmark to monitor performance for large dataset indexing (#9012) Zarr: Optimize `region="auto"` detection (#8997) Trigger CI only if code files are modified. (#9006) Fix for ruff 0.4.3 (#9007) Port negative frequency fix for `pandas.date_range` to `cftime_range` (#8999) Bump codecov/codecov-action from 4.3.0 to 4.3.1 in the actions group (#9004) Speed up localize (#8536) Simplify fast path (#9001) Add argument check_dims to assert_allclose to allow transposed inputs (#5733) (#8991) Fix syntax error in test related to cupy (#9000)

* backend-indexing: Trigger CI only if code files are modified. (pydata#9006) Enable explicit use of key tuples (instead of *Indexer objects) in indexing adapters and explicitly indexed arrays (pydata#8870) add `.oindex` and `.vindex` to `BackendArray` (pydata#8885) temporary enable CI triggers on feature branch Avoid auto creation of indexes in concat (pydata#8872) Fix benchmark CI (pydata#9013) Avoid extra read from disk when creating Pandas Index. (pydata#8893) Add a benchmark to monitor performance for large dataset indexing (pydata#9012) Zarr: Optimize `region="auto"` detection (pydata#8997) Trigger CI only if code files are modified. (pydata#9006) Fix for ruff 0.4.3 (pydata#9007) Port negative frequency fix for `pandas.date_range` to `cftime_range` (pydata#8999) Bump codecov/codecov-action from 4.3.0 to 4.3.1 in the actions group (pydata#9004) Speed up localize (pydata#8536) Simplify fast path (pydata#9001) Add argument check_dims to assert_allclose to allow transposed inputs (pydata#5733) (pydata#8991) Fix syntax error in test related to cupy (pydata#9000)

ignamv added 2 commits May 1, 2024 14:02

Add argument check_dims to assert_allclose to allow transposed inputs

cfd41ab

Update whats-new.rst

49c318c

ignamv added 3 commits May 1, 2024 15:22

Add check_dims argument to assert_equal and assert_identical + tests

c4e39f9

Assert that dimensions match before transposing or comparing values

9ea9148

Add docstring for check_dims to assert_equal and assert_identical

0ace755

TomNicholas reviewed May 1, 2024

View reviewed changes

TomNicholas added the topic-testing label May 1, 2024

Update doc/whats-new.rst

413b8bf

Co-authored-by: Tom Nicholas <tom@cworthy.org>

max-sixty reviewed May 1, 2024

View reviewed changes

xarray/testing/assertions.py Outdated Show resolved Hide resolved

max-sixty reviewed May 1, 2024

View reviewed changes

ignamv and others added 4 commits May 2, 2024 06:23

Undo fat finger

97ea491

Co-authored-by: Tom Nicholas <tom@cworthy.org>

Add attribution to whats-new.rst

ab677c2

Replace check_dims with bool argument check_dim_order, rename align_d…

de2d105

…ims to maybe_transpose_dims

Remove left-over half-made test

35c1838

max-sixty approved these changes May 4, 2024

View reviewed changes

ignamv and others added 7 commits May 5, 2024 11:41

Remove check_dim_order argument from assert_identical

18d18a9

assert_allclose/equal: emit full diff if dimensions don't match

3f14b21

Rename check_dim_order test, test Dataset with different dim orders

d558165

Update whats-new.rst

bfa5e92

Merge remote-tracking branch 'upstream/main' into assert_allclose_tra…

b8153a6

…nspose_dimensions

Hide maybe_transpose_dims from Pytest traceback

555a626

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

Ignore mypy error due to missing functools.partial.__name__

eb66fd2

max-sixty merged commit 8d728bf into pydata:main May 5, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add argument check_dims to assert_allclose to allow transposed inputs (#5733) #8991

Add argument check_dims to assert_allclose to allow transposed inputs (#5733) #8991

ignamv commented May 1, 2024 •

edited by TomNicholas

Loading

welcome bot commented May 1, 2024

TomNicholas left a comment

TomNicholas May 1, 2024

ignamv May 2, 2024

TomNicholas May 2, 2024

TomNicholas May 1, 2024

max-sixty May 1, 2024

ignamv May 2, 2024

max-sixty May 2, 2024

max-sixty May 1, 2024

max-sixty May 4, 2024

max-sixty commented May 5, 2024

welcome bot commented May 5, 2024

Add argument check_dims to assert_allclose to allow transposed inputs (#5733) #8991

Add argument check_dims to assert_allclose to allow transposed inputs (#5733) #8991

Conversation

ignamv commented May 1, 2024 • edited by TomNicholas Loading

welcome bot commented May 1, 2024

TomNicholas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-sixty commented May 5, 2024

welcome bot commented May 5, 2024

ignamv commented May 1, 2024 •

edited by TomNicholas

Loading