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

More informative error for passing a list to dataset.transpose #7120

Merged
merged 9 commits into from
Oct 4, 2022

Conversation

patrick-naylor
Copy link
Contributor

Closes #6502

  • 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

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.

Thank you v much @patrick-naylor

xarray/core/dataset.py Outdated Show resolved Hide resolved
xarray/core/dataset.py Outdated Show resolved Hide resolved
patrick-naylor and others added 2 commits October 3, 2022 18:56
Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
@max-sixty
Copy link
Collaborator

Here's a fix of the type checker and a small suggestion (feel free to accept or reject!)

commit 5dad26e19f1fb22e9a31854bac9471c5e1bec43c
Author: Maximilian Roos <m@maxroos.com>
Date:   Tue Oct 4 00:26:48 2022 -0700

    Ignore type on intentional error; (optional) tweak to error message

diff --git a/xarray/core/dataset.py b/xarray/core/dataset.py
index ff54bd84..a3f26d26 100644
--- a/xarray/core/dataset.py
+++ b/xarray/core/dataset.py
@@ -5405,7 +5405,7 @@ def transpose(
         if (len(dims) > 0) and (isinstance(dims[0], list)):
             list_fix = [f"'{x}'" if isinstance(x, str) else f"{x}" for x in dims[0]]
             raise TypeError(
-                f'transpose requires dims to be passed as multiple arguments. Expected "{", ".join(list_fix)}". Received "{dims[0]}" instead'
+                f'transpose requires dims to be passed as multiple arguments. Expected `.transpose({", ".join(list_fix)})`. Received `.transpose({dims[0]})` instead'
             )
 
         # Use infix_dims to check once for missing dimensions
diff --git a/xarray/tests/test_dataset.py b/xarray/tests/test_dataset.py
index 6ac5ed06..8acd3798 100644
--- a/xarray/tests/test_dataset.py
+++ b/xarray/tests/test_dataset.py
@@ -6809,7 +6809,7 @@ def test_string_keys_typing() -> None:
     ds.assign(variables=mapping)
 
 
-def test_traspose_error() -> None:
+def test_transpose_error() -> None:
     # Transpose dataset with list as argument
     # Should raise error
     ds = xr.Dataset({"foo": (("x", "y"), [[21]]), "bar": (("x", "y"), [[12]])})
@@ -6817,7 +6817,7 @@ def test_traspose_error() -> None:
     with pytest.raises(
         TypeError,
         match=re.escape(
-            "transpose requires dims to be passed as multiple arguments. Expected \"'y', 'x'\". Received \"['y', 'x']\" instead"
+            "transpose requires dims to be passed as multiple arguments. Expected `.transpose('y', 'x')`. Received `.transpose(['y', 'x'])` instead"
         ),
     ):
-        ds.transpose(["y", "x"])
+        ds.transpose(["y", "x"])  # type: ignore

@patrick-naylor
Copy link
Contributor Author

patrick-naylor commented Oct 4, 2022

Thanks @max-sixty

Here's a fix of the type checker and a small suggestion (feel free to accept or reject!)

commit 5dad26e19f1fb22e9a31854bac9471c5e1bec43c
Author: Maximilian Roos <m@maxroos.com>
Date:   Tue Oct 4 00:26:48 2022 -0700

    Ignore type on intentional error; (optional) tweak to error message

diff --git a/xarray/core/dataset.py b/xarray/core/dataset.py
index ff54bd84..a3f26d26 100644
--- a/xarray/core/dataset.py
+++ b/xarray/core/dataset.py
@@ -5405,7 +5405,7 @@ def transpose(
         if (len(dims) > 0) and (isinstance(dims[0], list)):
             list_fix = [f"'{x}'" if isinstance(x, str) else f"{x}" for x in dims[0]]
             raise TypeError(
-                f'transpose requires dims to be passed as multiple arguments. Expected "{", ".join(list_fix)}". Received "{dims[0]}" instead'
+                f'transpose requires dims to be passed as multiple arguments. Expected `.transpose({", ".join(list_fix)})`. Received `.transpose({dims[0]})` instead'
             )

This is what I initially had but I thought it might be confusing for users passing something to missing_dims

-def test_traspose_error() -> None:
+def test_transpose_error() -> None:
     # Transpose dataset with list as argument
     # Should raise error
     ds = xr.Dataset({"foo": (("x", "y"), [[21]]), "bar": (("x", "y"), [[12]])})
@@ -6817,7 +6817,7 @@ def test_traspose_error() -> None:
     with pytest.raises(
         TypeError,
         match=re.escape(
-            "transpose requires dims to be passed as multiple arguments. Expected \"'y', 'x'\". Received \"['y', 'x']\" instead"
+            "transpose requires dims to be passed as multiple arguments. Expected `.transpose('y', 'x')`. Received `.transpose(['y', 'x'])` instead"
         ),
     ):
-        ds.transpose(["y", "x"])
+        ds.transpose(["y", "x"])  # type: ignore

This is very helpful thank you!

@max-sixty
Copy link
Collaborator

Thanks a lot @patrick-naylor !

@max-sixty max-sixty enabled auto-merge (squash) October 4, 2022 18:08
@max-sixty max-sixty merged commit 13c52b2 into pydata:main Oct 4, 2022
@patrick-naylor patrick-naylor deleted the transpose-error branch October 4, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise nicer error if passing a list of dimension names to transpose
2 participants