issue_comments

Thanks again @shoyer for your reviews and comments!

Merging this PR broke the tests added in #881. I fixed that in #903.

@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.

Thanks for your second review @shoyer ! It actually helped me to better understand the indexing logic of pandas (never too late)!

I made some updates according to your comments.

I think we're getting closer to a working feature!

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

Noted the difference in documentation.

Hope that the whole "multi-level indexing" section is clear enough, not sure if it's proper English.

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.

That behavior looks good enough to me too! It is indeed already quite flexible :-).

Do you think that we really need to handle data.loc['a', 0] if data is 2 or higher dimensional? We would also have to handle the case data.loc[{'one': 'a', 'two': 0}]... Having to explicitly write data.loc[('a', 0), :] or data.loc[('a', 0), ...] seems fine to me (at least for now).

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.

```

index = pd.MultiIndex.from_product((list('abc'), range(3))) index.names = ('one', 'two') data = xr.DataArray(range(9), [('x', index)]) data <xarray.DataArray (x: 9)> array([0, 1, 2, 3, 4, 5, 6, 7, 8]) Coordinates: * x (x) object ('a', 0) ('a', 1) ('a', 2) ('b', 0) ('b', 1) ... ```

Example using a tuple of scalar/tuple/list/slice labels for each level:

```

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

Example using a list of several combinations of multi-index labels:

```

data.sel(x=[('a', 0), ('b', 1), ('c', 1)]) <xarray.DataArray (x: 3)> array([0, 4, 7]) Coordinates: * x (x) object ('a', 0) ('b', 1) ('c', 1) ```

All the following examples raise an error:

```

data.sel(x=(('a', 0), ('b', 1), ('c', 1))) data.sel(x=['a', (0, 1)]) data.sel(x=[['a', 0], ['b', 1], ['c', 1]]) ```

Note that after verification, pandas has the same behavior for all of the examples shown here.

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?

Thanks for the review @shoyer !

I made some updates according to your comments.

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)?

I finally managed to add some tests and docs.

Two more comments: - Index replacement: if the multi-index has unnamed levels, the chosen name for the replaced index (if any) is <old_dim>_unnamed_level (old_dim is the name of the replaced multi-index dimension). Not sure whether or not it is really necessary to track the index level number. Single indexes returned by get_loc_level don't keep that information. Anyway, it is always better to set explicit names for the index levels. - Indexing on multi-index now also works with nested tuples (#850).

@shoyer I think that it is ready for review. I successfully run the tests on my local branch. Currently, CI tests seem broken for some reason I don't know.

I hope to have some time next week to work again on this PR.

@tippetts You can see in #719 a few comments about saving/reading xarray data objects with multi-index to/from netCDF. I also looking forward to see this feature implemented - actually I need it for another project that uses xarray - so maybe I'll find some time in the next couple of weeks to start a new PR on this.

Unless you see any other issues, I think that this feature doesn't need more development for now.

I'll be back next week to finish this PR (write some tests and doc).

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

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.

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.

I followed your suggestions.

Two more comments (not critical issues I think) : - 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. - 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.

In summary, da.loc[{'band_wavenumber': {'band': 'foo'}}] returns a DataArray with updated index and coordinate / dimension name, da.loc['foo'] returns a DataArray with the full multi-index, and da.loc[{'band': 'foo'}] raises a KeyError. Three different results for the same operation, although I think that the first one is more explicit and better.

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.