issue_comments

Yes, we want to replace that test with your test — either in the same test function or a new test function.

Though it maybe it should still be a KeyError, I think probably it should be. But feel free to submit the PR and it's easiest to review there.

Thanks!

here's an example: python In [2]: xr.Dataset({"a": ([("a", "b")], [1])}) Out[2]: <xarray.Dataset> Dimensions: (('a', 'b'): 1) Dimensions without coordinates: ('a', 'b') Data variables: a (('a', 'b')) int64 1

If you allow explicitly tuple, doesn't it mean I need to edit this part

Yes!

(I find it a little bit weird to give dimensions in list, no ? I thought that generally tuples were more adapted to provide dims)

Yes, I feel similarly. For one xr.Dataset(dict(var=(('a','b'), np.random.rand(3)))) is an error, rather than a one-dimensioned dataset with a single dim called ('a','b'). But OTOH I can very much empathize with accepting any Hashable for dim names most functions, which includes tuples. So I am fine formalizing this (apart from the constructor).

Can you tell me which test-case I can implement to fulfill all the requirements needed ? I can't think of test cases that I can implement except the one suggested first, and that one passes with the first modification

How about a test that checks the example above suggests a list, rather than raising a vanilla KeyError?

Cheers @thomashirtz

Great, agree @dcherian . So maybe we: - Check whether the dim exists (there could be a dim with tuple), which is already done in 1371 below - If it doesn't exist, then check whether it's a string before passing it to _get_virtual_variable below. If it's a tuple, that message can suggest passing it as a list. - Or we do the check in _get_virtual_variable?

python 1366 def _construct_dataarray(self, name: Hashable) -> "DataArray": 1367 """Construct a DataArray by indexing this dataset""" 1368 from .dataarray import DataArray 1369 1370 try: 1371 variable = self._variables[name] 1372 except KeyError: -> 1373 _, name, variable = _get_virtual_variable( 1374 self._variables, name, self._level_coords, self.dims 1375 )

@thomashirtz changing the labels back! Thanks in advance!

I think 2 and 3 become really confusing with swap_dims (for e.g.) So my vote is for 1. In all three cases, the solution to this issue is to suggest passing lists in the error message?

Any thoughts from others on what we should do, of the three options above? CC @pydata/xarray

All of these options are breaking changes so we would probably have to go through a deprecation cycle. 1 might be the most consistent and least confusing, but if we want to keep the current behavior we could require wrapping a tuple in another sequence to access dimension names.

I would propose we either: - Explicitly allow tuples as dimension and variable names. This is probably the most mypy-compatible, since we use Hashable in lots of places - Force dimension names to be strings, but allow variable names to be Hashable. This might be the most practical. - Disallow tuples for dimension names, but otherwise allow Hashable. Then tuples work the same as lists when supplied as arguments.

Ah yes you are right... However, we are not consistent with this, e.g. that works for reductions air.mean(("lat", "lon"))

I think it's the way it is because tuple is hashable, so it can be used as a name: python ds = xr.Dataset({("a", 0): ("x", [1, 0])}) ds.sortby(("a", 0)) not sure if there's a particular use case where supporting this is crucial

Yes, I am marking that as a bug. The problem is here:

https://github.com/pydata/xarray/blob/295606707a0464cd13727794a979f5b709cd92a1/xarray/core/dataset.py#L5612-L5613

as the tuple is not a list it ends up as [('x', 'y')]. That should probably be python if isinstance(variables, str): variables = [variables] or python if not isinstance(variables, (list, tuple)): variables = [variables]