issue_comments

Do we need to change the behavior of dict(dataset) so that dict(dataset).keys() and dataset.keys() become consistent?

No, I think these are guaranteed to be consistent because we inherit from collections.Mapping to implement dict methods like keys(), values() and items() (via __iter__ and __getitem__).

Another question is what to do with x in ds (i.e.,Dataset.__contains__). Currently it checks data variables and coordinates, but not dimensions (as @fujiisoup pointed out in https://github.com/pydata/xarray/pull/1632#issuecomment-336637245), which already feels somewhat inconsistent.

If we were starting over, I might change how __contains__ works for Dataset to only include data variables, but I don't think it's worth it at this time:

• Unlike the current version of __iter__, __contains__ is actually useful in its current state and I suspect is widely used. Issuing a deprecation warning for 'variable' in ds would annoy lots of users, for no particularly good reason. In many cases (checking data variables), the behavior would not actually change.
• There is also the expectation that k in ds is equivalent to ds[k] for mapping types, which is currently mostly true for xarray.Dataset.
• The main advantage I see to changing the behavior of k in ds is that it would remove most of the remaining use cases for Dataset.data_vars, which would let us eventually remove data_vars. But given that ds[k] for coordinates k will continue to be supported, I think I would still recommend explicitly writing k in ds.data_vars for cases where the data/coordinates distinction matters.

Related: over in https://github.com/pydata/xarray/pull/1645, I am deprecating the current behavior of DataArray.__contains__, so that we can make it check array values instead of DataArray.coords in the future.

If we aren't going to fundamentally change the behavior of k in ds to exclude coordinates, then we should probably update it, at @fujiisoup suggests, to also include MultiIndex levels and dimension names (i.e., the stuff checked in xarray.core.dataset._get_virtual_variable()).

I'm pretty sure now that we should (at least) change iter(ds) and ds.keys() to only include data variables. This is a repeated source of annoyance.

For the most recent example, consider the API for argmin() suggested by @fujiisoup in https://github.com/pydata/xarray/issues/1388#issuecomment-338397633, where argmin() would return a Dataset. We'd like make arr.isel(**arr.argmin(dim)) work, but this requires no additional members (beyond data variables) in arr.argmin(dim).keys().

The question is how to do it: is it worth a deprecation cycle? My proposal: - In v0.10, start issuing FutureWarning when calling ds.__iter__. Suggest that users switch to iterating over .variables instead if they really want everything. (Many cases where every data variable and coordinate is being iterated over should probably already be using the low-level API.) - In v0.11, switch to the new behavior.

Note: __len__ should also be changed in lock-step, to ensure the invariant len(list(ds)) == len(ds).

What would happen to the actual variables? Would ds['longitude'] also return a KeyError? This would be very far from the NetCDF model and would brake many many things....

There is no way we would change this -- ds['longitude'] will always return the longitude array.

What are your thoughts now?

I like this change for the reasons you outline above, and those I mentioned in the previous issue. I'm somewhat concerned about breaking a lot of user code, and also am not sure yet what the right solution here looks, because of concerns about breaking duck-type compatibility with dictionaries.

Which of these should no longer include coordinates? Can we make things consistent without changing all of them? - iter(ds) (currently we actually define Dataset.keys() implicitly via __iter__) - 'x' in ds - del ds['x'] - ds['x'] (this would be quite a compatibility break, as @fmaussion notes above)

We would also need to encourage users to use the low level Dataset.variables property to check whether something is either a coordinate or data variables.

Here is one place where I discussed this with myself! https://github.com/pydata/xarray/issues/211

Then I merged the functionality you are describing, but at some point switched it back....

An early version of xarray actually worked exactly like this, but we switched it to the current functionality. Let me see if I can dig up the relevant issues....