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

Consider accepting a positional dict argument in place of keyword arguments #1231

Closed
shoyer opened this issue Jan 26, 2017 · 2 comments
Closed

Comments

@shoyer
Copy link
Member

shoyer commented Jan 26, 2017

Using **kwargs in xarray operations is incredibly convenient, but it means there is no way to use a keyword argument that conflicts with an existing argument name. For example, there is no way to use .sel to select along 'method', 'tolerance' or 'drop' dimensions.

For interactive use, this is fine -- these reserved names are rarely used and it's nice to save the keystrokes it takes to write dict(). But it's a problem for writing reliable complex software, which may be calling xarray methods from somewhere very different. In fact, knowing how this works, it's possible to trigger bugs without even using **kwargs, e.g., by indexing a DataArray:

In [34]: array = xr.DataArray([1, 2, 3], dims='drop')

# result should be a scalar!
In [35]: array[0]
Out[35]:
<xarray.DataArray (drop: 3)>
array([1, 2, 3])
Unindexed dimensions:
    drop

One option to resolve this is to make the first argument to every function like this (including sel, isel, sel_points, isel_points, stack, ...) accept a dictionary, which is interpreted exactly like **kwargs. In fact, we already do this in a highly inconsistent fashion for Dataset.reindex only. For all cases where axis names are set dynamically (and certainly within xarray), we could encourage using the dictionary form, e.g., array.isel(kwargs, drop=True) rather than array.isel(drop=True, **kwargs).

@stale
Copy link

stale bot commented Jan 24, 2019

In order to maintain a list of currently relevant issues, we mark issues as stale after a period of inactivity
If this issue remains relevant, please comment here; otherwise it will be marked as closed automatically

@stale stale bot added the stale label Jan 24, 2019
@max-sixty
Copy link
Collaborator

I missed this issue and last year created #2188. I'll leave that one open as it has tracking
The good news is that it's mostly complete, mainly due to @fujiisoup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants