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!

Ok, I think I understand better now Almost finished, I just have one test that fails currently, it is this one: https://github.com/pydata/xarray/blob/e17cf595c84dffdd73c668f6d945c1b0eeba13d6/xarray/tests/test_dataset.py#L3299-L3306

The error changed from a KeyError to the error that I wrote in the constructor : ```

          raise TypeError(f'The dimension provided is a tuple, you may intended to pass a list')

E TypeError: The dimension provided is a tuple, you may intended to pass a list ```

I am not sure what I should do with it, should I delete it ?

(Thanks for your help :))

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

I think I am quite confused by the fact that 'one' dimension can be a tuple (also I couldn't find a way to successfully create a dataset with a dim being a tuple, even by tweaking your example using list)

Was it what you imagined for the dataset construction ? (If I understand right, a virtual variable can't be a tuple) ``` def _construct_dataarray(self, name: Hashable) -> "DataArray": """Construct a DataArray by indexing this dataset""" from .dataarray import DataArray

    try:
        variable = self._variables[name]
    except KeyError:
        if isinstance(name, tuple):
            raise TypeError(f'The dimension "{name}" provided is a tuple, you may be intended to pass it as a list')
        else:
        _, name, variable = _get_virtual_variable(
            self._variables, name, self._level_coords, self.dims
        )

```

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

You seemed to all agree on the solution 1:

Explicitly allow tuples as dimension and variable names. This is probably the most mypy-compatible, since we use Hashable in lots of places

If you allow explicitly tuple, doesn't it mean I need to edit this part (as mentioned by @mathause) : if not isinstance(variables, (list, tuple)): variables = [variables] (I tried and it works well)

But you said after :

If it's a tuple, that message can suggest passing it as a list.

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

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

Also the code you mentioned is for one dim, but in this case we can have a list of dims, it does means that I need to do a 'for loop' for checking the existence of the dims, right ?

@thomashirtz Feel free to work on it.

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?

(@max-sixty , I originally took this one because I wanted to do one more ~easy PR. I just saw you modified the tags, lmk if this one is problematic or if you know other issues I could tackle)

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

@mesejo Do you still work on it ? If not, I am interested to work on it

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.

I would like to work on this

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]