Skip to content

Provide better error message when dimension name matches argument #3336

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Provide better error message when dimension name matches argument #3336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion xarray/core/dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1999,7 +1999,11 @@ def sel(
Dataset.isel
DataArray.sel
"""
indexers = either_dict_or_kwargs(indexers, indexers_kwargs, "sel")
func_args = set(locals().keys())
dims = set(self.dims)
indexers = either_dict_or_kwargs(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the magic is OK for the convenience; but is there a better way than locals()? Like inspect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this works

func_args = set(inspect.signature(self.sel).parameters)

and is less brittle than locals() without much function-specific overhead. either_dict_or_kwargs could even take self.sel as an argument, and call inspect itself. I did a basic performance test, and locals() is 1/100th of a second faster on average.

That said, do we want to not raise an error on something like this?

arr = xr.DataArray(np.random.random((100, 10)),
                   dims=['foo', 'method'],
                   coords={
                       'foo': range(100),
                       'method': range(10)
                   })
 arr.sel(foo=range(0, 100, 2))

My current implementation will raise an error because while the user did not pass in a value for method, it is still a function parameter. One way to address this is something like

locals_ = locals()
params = inspect.signature(self.sel).parameters
func_args = {p for p in params if locals_[p] != params[p].default}

but now we're getting even more complicated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what to do here. Perhaps @crusaderky has some ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it's seriously overengineered to me. You really shouldn't be using inspect.signature when you're dealing with a single, hardcoded function...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the idea is to use a helper function anytime we allow dict or kwargs. sel is the test case for now

indexers, indexers_kwargs, "sel", func_args=func_args, dims=dims
)
pos_indexers, new_indexes = remap_label_indexers(
self, indexers=indexers, method=method, tolerance=tolerance
)
Expand Down
9 changes: 9 additions & 0 deletions xarray/core/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,16 @@ def either_dict_or_kwargs(
pos_kwargs: Optional[Mapping[Hashable, T]],
kw_kwargs: Mapping[str, T],
func_name: str,
func_args: Any = None,
dims: Any = None,
) -> Mapping[Hashable, T]:
if func_args is not None:
inter = func_args.intersection(dims)
if inter:
raise ValueError(
"the dimension name '%s' matches an argument "
"to .%s" % (inter.pop(), func_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Please pass a dictionary to avoid this conflict." or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will do.

)
if pos_kwargs is not None:
if not is_dict_like(pos_kwargs):
raise ValueError(
Expand Down