Improved default behavior when concatenating DataArrays

[Zac-HD]

• Closes Improved inference of names when concatenating arrays #2775
• Tests added
• Fully documented, including whats-new.rst for all changes and api.rst for new API

This is really nice to have when producing faceted plots of satellite observations in various bands, and should be somewhere between useful and harmless in other cases.

Example code:

ds = xr.Dataset({
    k: xr.DataArray(np.random.random((2, 2)), dims="x y".split(), name=k) 
    for k in "blue green red".split()
})
xr.concat([ds.blue, ds.green, ds.red], dim="band").plot.imshow(col="band")

Before - facets have an index, colorbar has misleading label:

After - facets have meaningful labels, colorbar has no label:

[shoyer]

This seems like a nice usability improvement!

[Zac-HD]

Thanks for the support and quick review @shoyer!

Any idea when Xarray 0.12 might be out? I'm teaching some remote sensing workshops in mid-March and would love to have this merged, as a colleague's review of those notebooks prompted this PR 😄

[Zac-HD]

The docs build failed due to a (transient) http error when loading tutorial data for the docs, so I've also finalised the planned conversion from xarray.tutorial.load_dataset to xarray.tutorial.open_dataset.

[Zac-HD]

Hmm, it looks like the failure to download the naturalearth coastlines.zip wasn't so transient after all - but it does work on my machine!

[Zac-HD]

OK! @shoyer, I've got everything passing and it's ready for review.

Even the accidental tutorial/docs fixes 😄

[shoyer]

I'll check on the doc build failure, but I would rather not remove load_dataset yet -- we didn't deprecate it that long ago.

…

On Fri, Feb 22, 2019 at 5:28 AM Zac Hatfield-Dodds ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In xarray/core/combine.py <#2777 (comment)>: > - elif name != arr.name: - if compat == 'identical': - raise ValueError('array names not identical') - else: - arr = arr.rename(name) - datasets.append(arr._to_temp_dataset()) + name = result_name(arrays) + names = [arr.name for arr in arrays] + if compat == 'identical' and len(set(names)) != 1: + raise ValueError( + "compat='identical', but array names {!r} are not identical" + .format(names if len(names) <= 10 else sorted(set(names))) + ) + datasets = [arr.rename(name)._to_temp_dataset() for arr in arrays] + + if isinstance(dim, str) and len(set(names) - {None}) == len(names) \ Not for this - we're checking that names is a list of unique strings. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2777 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKS1nbwyPzYE3HzT30pWhWignDUrbBSks5vP_B_gaJpZM4bCN3h> .

[Zac-HD]

@shoyer - don't worry about the docs build, I'm pretty sure that was just a flaky network from Travis and it's working now in any case.

I've left tutorial.load_dataset in, just changed "removed in 0.12" to "removed in a future version".

[shoyer]

This looks good to me now. I'll merge in a day or two unless anyone else has review comments to add.

[pep8speaks]

Hello @Zac-HD! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 27, 2019 at 00:51 Hours UTC

[shoyer]

@pep8speaks seems to have gone hay-wire -- maybe you have a syntax error?

Thinking about this a little more, one hazard of converting names into index labels is that we lose the invariant that you get the same result regardless of order in which you call concat, e.g., something like these expressions could now give different results:

xarray.concat([a, b], dim='x')

vs

xarray.concat([xarray.concat([a], dim='x'), xarray.concat([b], dim='x')], dim='x')

If a and b have the same name, then you'd get an index with the two duplicate entries in the second case but not the first case.

I'm not entirely sure this is a deal-breaker but it makes me a little nervous reluctant. In particular, it might break some the invariants we're relying upon for the next version of open_mfdataset (#2616, cc @TomNicholas )

[Zac-HD]

maybe you have a syntax error?

...yep, an unmatched paren. 😥

If a and b have the same name, then you'd get an index with the two duplicate entries in the second case but not the first case. [which would be bad]

I think it's impossible to avoid this when using inference in the general case. Two options I think would be decent-if-unsatisfying:

1. Explicitly manage this in the new combining functions, e.g. clear the concat dim coords if they are not unique and the input arrays did not have coords in that dimension.
2. Add an argument to xr.concat to enable or disable this, e.g. infer_coords=True, and disable it when calling xr.concat from other combining functions.

[TomNicholas]

@Zac-HD forgive me for this but I think this PR is unnecessary because what you need basically already exists in the API.

Going back to your original example, you could have got the same indexing by creating a DataArray to use as a coordinate to concatenate over:

colors = "blue green red".split()
ds = xr.Dataset({
    k: xr.DataArray(np.random.random((2, 2)), dims="x y".split(), name=k)
    for k in colors
})

band = xr.DataArray(colors, name="band", dims=["band"])
xr.concat([ds.blue, ds.green, ds.red], dim=band).plot.imshow(col="band")

This still leaves the wrong label on the colorbar, but that could be fixed separately and has to do with concat using the attrs of the first dataset in the list for the final dataset (a similar problem to #2382). I think it would be easier to change that behaviour instead (perhaps to if all names the same, use that name, else name of result = None, but this also relates to #1614).

Creating a new coordinate using a DataArray is in the docstring for xr.concat:

If dimension is provided as a DataArray or Index, its name is used as the dimension to concatenate along and the values are added as a coordinate.

but I think there should be an example there too. (Also I think this is relevant to #1646)

I'm not entirely sure this is a deal-breaker but it makes me a little nervous

@shoyer I agree, although I like the idea then I think this could introduce all sorts of complex concatentation edge cases.

At the very least the new API should have symmetry properties something like:

da1 = DataArray(name='a', data=[[0]], dims=['x', 'y'])
da2 = DataArray(name='b', data=[[1]], dims=['x', 'y'])
da3 = DataArray(name='a', data=[[2]], dims=['x', 'y'])
da4 = DataArray(name='b', data=[[3]], dims=['x', 'y'])

xr.manual_combine([[da1, da2], [da3, da4]], concat_dim=['x', 'y'])
# should give the same result as 
xr.manual_combine([[da1, da3], [da2, da4]], concat_dim=['y', 'x'])

but with this PR I don't think it would. In the first case the x coord would be created with values ['a', 'b'], and no y coord would be created, while in the second case no y coord would be created, and the intermediate DataSet would be nameless, so then no x coord would be created either.

I think my suggestion for naming would pass this test because the result would be nameless and have no coords either way.

I might have got that wrong but I definitely think this kind of change should be carefully considered 😕

(EDIT: I just added this example as a test to #2616)

[TomNicholas]

I've just submitted a PR which solves this issue in the way I just suggested instead #2792.

[TomNicholas]

@Zac-HD there's actually another way to get the indexing behaviour you wanted with the current API:

colors = "blue green red".split()
das = [xr.DataArray(np.random.random((2, 2)), dims="x y".split(),
                    coords={"band": k})
       for k in colors]
xr.concat(das, dim="band").plot.imshow(col="band")

Here instead of using the name attribute to label each band I've used a scalar coordinate called "band", so that when you concat along "band" it will just stack along that coordinate.

This never touches the names so actually gives the desired output without the need for #2792:

[shoyer]

I guess we should probably roll back the "name to scalar coordinates" part of this change.

@Zac-HD do you want to do that here or should we go with @TomNicholas's PR?

[Zac-HD]

No objection to going with #2792; I'm just happy to have the change merged 😄

It would be nice for someone to cherry-pick 63da214 before releasing 0.12 though, just to fix that warning.