Thanks Joe!

Kudos to @nbren12 for sticking with this one! Looking forward to seeing this one in the wild.

phew! Thanks for all the reviews and discussion everyone!

Thanks @nbren12!

It looks like the CI errors above aren’t related to this PR. There seems to be an issue with to_pandas.

Hello @nbren12! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2019-07-04 20:50:20 UTC

Codecov Report

Merging #1597 into master will increase coverage by 0.02%. The diff coverage is 97.22%.

```diff @@ Coverage Diff @@

master #1597 +/-

========================================== + Coverage 94.8% 94.83% +0.02%
========================================== Files 67 67
Lines 12876 12912 +36
========================================== + Hits 12207 12245 +38
+ Misses 669 667 -2 ```

OK, I plan to merge this shortly unless there are any objections :)

drat. Should be fixed now.

you have a lint error: https://github.com/pydata/xarray/pull/1597#issuecomment-478700651

Okay. I responded to @benbovy's last comments and merged the upstream changes to master. How is this looking?

cc @jhamman

is there actual use cases where it is useful to provide multiple dimensions for sample_dims?

Yes! My main use-case does. For example, if you have a weather dataset where every lat/lon pair should be considered a separate "sample", you would use sample_dims=['lat', 'lon'].

It looks like the tests passed. @benbovy How does it look now? Did I fix the issues you mentioned?

It looks like 43834ac8186a851b7 might have fixed this. Let's see if the tests pass.

Does anybody have an idea why the py36-dask-dev tests are failing? None of the errors seems related to this PR.

@benbovy Paradoxically I think making to_stacked_array accept the list of dimensions which won't be stacked is clearer. These unstacked dims needed to shared across all variables, which is a very simple requirement. I realized this after writing a bunch of error-prone validation code. See my last commit for more of my reasoning (5ca9a1d868a). I also updated the docs and tests accordingly.

@benbovy

Is it expected behavior? I guess that not every combination is possible because we're not broadcasting here unlike to_array().

I believe so. In your third case, to_stacked_array basically wants to combine a and b into a DataArray with a dimension y. But that is not possible without broadcasting because b does not have a dimension y. I will try to add a more informative error message.

Thanks for your review @benbovy. I'll try to take a look soon.

docs & tests LGTM. I don't understand stacking/unstacking/multiindexes too well...

@pydata/xarray - would love to get some more eyes on this PR. Its been lingering but @nbren12 seems to have addressed all prior comments.

Hey @rabernat. Did I resolve your comments?

@rabernat - you had asked for some changes in the early days of this PR. Are you satisfied with the current state here?

Hey. I just wanted to bump this PR. How does it look @rabernat @jhamman @shoyer?

Ok. I added an example which is very similar to the DataArray.unstack example.

Okay. It looks like it is passing CI now, so I think it's ready for another look. How does it look? Are you still interested in including this functionality?

No worries, rebase is fine too :)

On Mon, Apr 1, 2019 at 2:09 PM Noah D Brenowitz notifications@github.com wrote:

@shoyer https://github.com/shoyer Sorry. I rebased...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1597#issuecomment-478748969, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1g66J1a8H0wf1DzYQkEfqqIGGuc2ks5vcnWMgaJpZM4PmdUM .

@shoyer Sorry. I rebased...

@jhamman I did a little more work on this today. How do you recommend I update this to master? rebase?

We should add a section to our contributor guide on this, but the recommended workflow is just to use git merge.

@jhamman I did a little more work on this today. How do you recommend I update this to master? rebase?

I'd be happy to pick this up again if you think it will go through.

@nbren12 - can we revive this? There or some conflicts but I think this was pretty close to done.

Sorry for random activity. I accidentally hard reset the master branch nbren12/xarray to pydata/xarray.

Could not review pull request. It may be too large, or contain no reviewable changes.

@nbren12 OK, sounds good to me. Yes, it's a good idea to put more speculative stuff in other packages. I will take a look at your actual implementation.

@shoyer If you are okay with it, I think we might want to leave that to a later date if ever. I am not exactly sure what a useful API for that would be ATM. On the other hand, I have been using the original stack_cat and unstack_cat functions for a couple of months, and they have handled my basic uses pretty well.

For more complicated uses (e.g. taking different subsets of each variable and concatenating the output), I have started working on a project which is similar to sklearn-pandas. Since there are a million ways several xarray variables could be processed/subsetted, stacked and then concatenated, I think this functionality should probably remain in a third party package for now.

@nbren12 in the linked issue (https://github.com/pydata/xarray/issues/1317#issuecomment-337796691) you wrote:

After using my own version of this code for the past month or so, it has occurred to me that this API probably will not support stacking arrays of with different sizes along shared arrays. For instance, I need to "stack" humidity below an altitude of 10km with temperature between 0 and 16 km. IMO, the easiest way to do this would be to change these methods into top-level functions which can take any dict or iterable of datarrays. We could leave that for a later PR of course.

Is this something that would make sense to resolve before we merge this?

Okay. I think I'm done with the updates to the documentation.

I just added some docs, but need to add to whats-new still,

@nbren12 - can you add these methods to api.rst and add a note to whats-new (in addition to adding some docs to the reshaping page)?

@rabernat Your point is well taken. I will add some docs/motivation to the reshaping page.

Okay. I just changed the names of the methods and wrote a test case for the problem with the dtype of the stacked dimensions not being preserved by to_stacked_array.

At the moment, I am filling in the missing dimensions with None, so the resulting index has dtype object'. This is then concatenated with the bona fide indices of the variables which are not missing this index, so the index as a whole is cast to the lowest common denominator, which is object. Unfortunately we can't just put Nan in because, NaN is a floating point number thing, so I don't think all numpy dtypes have the equivalent of NaN. @shoyer Do you have any thoughts about how to resolve this?

That naming sounds good to me.

Also, I was having an issue with unstack_cat returning an index with dtype object that I would like to sort out.

If we have a separate method for to_stacked_array(), we should have a separate method for unstacking, too.

I like DataArray.to_unstacked_dataset() as the complement to Dataset.to_stacked_array(). I agree that it should be a DataArray method for chain-ability.

Or maybe DataArray.to_dataset_unstacked is good.

Thanks! Yah...I'm not very good at naming things. I think Dataset.to_stacked_array makes sense, but I would typically expect something like from_stacked_array to be a static method of Dataset (e.g. pd.MultiIndex.from_tuples). We could always move unstack_cat to Dataset, but then we miss out on the ability to make sequential method calls.

IMO the behavior of unstack_cat is kind of what I would expect DataArray.to_dataset to do in the case where dim is a stacked coordinate, so maybe we could put it in there.

I want to let @shoyer and @rabernat weigh in on the API here.