issue_comments

@tippetts Thanks for checking in. I actually hadn't noticed the last round of changes here, but I think this is good to go in now! Big thanks to @benbovy!!!

I did a little bit of testing. Here is one case I found where things don't work as I expected:

``` In [32]: idx = pd.MultiIndex.from_product([['a', 'b'], [0, 1, 2]], names=['lev0', 'lev1'])

In [33]: data = xr.DataArray(range(6), [('x', idx)])

In [34]: data.sel(x=('a',)) Out[34]: <xarray.DataArray (lev1: 3)> array([0, 1, 2]) Coordinates: * lev1 (lev1) int64 0 1 2

In [35]: data.sel(x='a') Out[35]: <xarray.DataArray (x: 3)> array([0, 1, 2]) Coordinates: * x (x) object ('a', 0) ('a', 1) ('a', 2) ```

I would expect an index drop in the last case, too. I guess we need to check for scalars.

Just went through and gave another full review -- this is looking quite nice, just a few more things to clean up!

I don't think we need to handle it -- I'm definitely supportive of only supporting unambiguous syntax. Let's just note the difference from pandas.

Yeah, that behavior looks pretty good to me -- there are limits on how flexible you can make things :).

We are fortunate that we have .sel, which lets us side-step some tricky ambiguous cases. Still, how do we handle data.loc['a', 0] if data is 2-dimensional? Pandas uses some terrible fall-back logic to allow the 0 to index either the second level or the second dimension, depending upon which works.

A possible remaining issue is that using a tuple of tuples, lists or slices exactly behaves like pandas (it calls get_locs) while a nested list can still be used to select couples of multi-index labels (it calls get_indexer). Should we allow only one of two behaviors, or keep both (and mention it in the docs)?

Can you give a quick example of what using the nested list look like?

Dictionaries not hashable, so we might be able to detect this case by calling hash() and raising a KeyError with alternative suggestions. Just an idea -- not sure if there's a clean way to implement that.

On Fri, Mar 25, 2016 at 10:10 AM, Benoit Bovy notifications@github.com wrote:

After refactoring a bit, da.loc[{'band': 'foo'}, :] and da.loc[{'band': 'foo'}, ...] now work as expected.

da.loc[{'band': 'foo'}] still raises a KeyError. Although it is possible to catch this exception, it still remains ambiguous so it is probably better to do nothing.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/pydata/xarray/pull/802#issuecomment-201370448

Multi-index level drop only works when a dict is provided for a dimension, i.e., da.sel(band_wavenumber='foo') still returns the full pandas.MultiIndex. This would require for indexing.convert_label_indexer to return an updated index also when label is not dict-like, which is not that straightforward I think.

Indeed. This would require an another data structure somewhere keeping track of level names -- and ideally also ensuring that they are always unique (like dimensions). This seems fine to me for now.

There is a potential conflict when providing a dictionary to DataArray.loc, it may be interpreted either as a mapping of dimensions / labels or a mapping of index levels / labels for the 1st dimension. For now, the second option is not handled, e.g., da.loc[{'band': 'foo'}] raises a KeyError.

I agree -- better to require the user to be explicit. I also don't see many use cases for specifying the coordinate value and level name but not the dimension name. What happens if you type da.loc[{'band': 'foo'}, :] or da.loc[{'band': 'foo'}, ...]? In principle these are not ambiguous, but I could only guess at what happens without looking at the code.

This looks very nice! I would probably opt for making drop_level=True the default for .loc -- it's hard to see when that wouldn't be desirable. In fact, I'm not even sure that it's worth having the option to set drop_level=False.