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

DataArray transpose inconsistent with Dataset Ellipsis usage #4647

Closed
ahuang11 opened this issue Dec 3, 2020 · 7 comments · Fixed by #4767
Closed

DataArray transpose inconsistent with Dataset Ellipsis usage #4647

ahuang11 opened this issue Dec 3, 2020 · 7 comments · Fixed by #4767

Comments

@ahuang11
Copy link
Contributor

ahuang11 commented Dec 3, 2020

This works:

import xarray as xr
ds = xr.tutorial.open_dataset('air_temperature')
ds.transpose('not_existing_dim', 'lat', 'lon', 'time', ...)

This doesn't (subset air):

import xarray as xr
ds = xr.tutorial.open_dataset('air_temperature')
ds['air'].transpose('not_existing_dim', 'lat', 'lon', 'time', ...)

The error message is a bit inaccurate too since I do have Ellipsis included; might be related to two calls of: dims = tuple(utils.infix_dims(dims, self.dims))


ValueError: ('not_existing_dim', 'lat', 'lon', 'time') must be a permuted list of ('time', 'lat', 'lon'), unless `...` is included

Traceback
...
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-5-793dfc1507ea> in <module>
      2 ds = xr.tutorial.open_dataset('air_temperature')
      3 ds.transpose('not_existing_dim', 'lat', 'lon', 'time', ...)
----> 4 ds['air'].transpose('not_existing_dim', 'lat', 'lon', 'time', ...)

~/anaconda3/envs/py3/lib/python3.7/site-packages/xarray/core/dataarray.py in transpose(self, transpose_coords, *dims)
   2035         if dims:
   2036             dims = tuple(utils.infix_dims(dims, self.dims))
-> 2037         variable = self.variable.transpose(*dims)
   2038         if transpose_coords:
   2039             coords: Dict[Hashable, Variable] = {}

~/anaconda3/envs/py3/lib/python3.7/site-packages/xarray/core/variable.py in transpose(self, *dims)
   1388         if len(dims) == 0:
   1389             dims = self.dims[::-1]
-> 1390         dims = tuple(infix_dims(dims, self.dims))
   1391         axes = self.get_axis_num(dims)
   1392         if len(dims) < 2 or dims == self.dims:

~/anaconda3/envs/py3/lib/python3.7/site-packages/xarray/core/utils.py in infix_dims(dims_supplied, dims_all)
    724         if set(dims_supplied) ^ set(dims_all):
    725             raise ValueError(
--> 726                 f"{dims_supplied} must be a permuted list of {dims_all}, unless `...` is included"
    727             )
    728         yield from dims_supplied
@dcherian
Copy link
Contributor

dcherian commented Dec 3, 2020

ds.transpose('not_existing_dim', 'lat', 'lon', 'time', ...)

IMO this should raise an error too

@ahuang11
Copy link
Contributor Author

ahuang11 commented Dec 3, 2020

ds.transpose('not_existing_dim', 'lat', 'lon', 'time', ...)

IMO this should raise an error too

I actually like it handling non_existing_dims automatically; maybe could be keyword though:
ds.transpose('not_existing_dim', 'lat', 'lon', 'time', ..., errors='ignore')

@dcherian
Copy link
Contributor

dcherian commented Dec 7, 2020

Yes i think missing_dims="ignore" would be great. This would match the kwarg in isel (https://xarray.pydata.org/en/stable/generated/xarray.Dataset.isel.html)

@mesejo
Copy link
Contributor

mesejo commented Jan 2, 2021

Hey! Can I work on this?

@ahuang11
Copy link
Contributor Author

ahuang11 commented Jan 2, 2021 via email

@mesejo
Copy link
Contributor

mesejo commented Jan 5, 2021

Hey, I was working on this and notice that transpose uses the method infix_dims to resolve the Ellipsis, should I put the expected behavior on infix_dims or only on transpose? The method infix_dims is also used in variable.transpose and in variable._stack_once. For me it seems right to put the new behavior on infix_dims to keep the behavior uniform, but I would like to know your opinion.

As a side-note I also noted that the return value of infix_dims is an iterator but on every usage is either converted to tuple or list, should I change the return value or keep it as it is?

@max-sixty
Copy link
Collaborator

Yes, agree this should be changed in infix_dims.

Fine to update the return type if it changes, but no need to coerce it premptively.

Thanks!

mesejo added a commit to mesejo/xarray that referenced this issue Jan 5, 2021
…psis usage

- Add missing_dims parameter to transpose to mimic isel behavior
- Add missing_dims to infix_dims to make function consistent
across different methods.
mesejo added a commit to mesejo/xarray that referenced this issue Jan 5, 2021
…psis usage

- Add missing_dims parameter to transpose to mimic isel behavior
- Add missing_dims to infix_dims to make function consistent
across different methods.
mesejo added a commit to mesejo/xarray that referenced this issue Jan 5, 2021
…psis usage

- Add missing_dims parameter to transpose to mimic isel behavior
- Add missing_dims to infix_dims to make function consistent
across different methods.
mesejo added a commit to mesejo/xarray that referenced this issue Jan 5, 2021
…psis usage

- Add missing_dims parameter to transpose to mimic isel behavior
- Add missing_dims to infix_dims to make function consistent
across different methods.
max-sixty added a commit that referenced this issue Jan 5, 2021
…sage (#4767)

- Add missing_dims parameter to transpose to mimic isel behavior
- Add missing_dims to infix_dims to make function consistent
across different methods.

Co-authored-by: Maximilian Roos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants