issue_comments

@shoyer I just re-encountered this bug whilst doing actual work - I would like to get it fixed, but need your input. In particular on

Do we want consistency with arithmetic, or consistency with merge?

(if this is particularly complex we could perhaps discuss it in the bi-weekly meeting?)

Also separately I think I've discovered a related weird bug: python da1 = xr.DataArray([0], name='a') da2 = xr.DataArray([1], name='b') xr.combine_by_coords([da1, da2]) returns /xarray/xarray/core/dataarray.py:682: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison return key in self.data /xarray/xarray/core/dataarray.py:682: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison return key in self.data Out[5]: <xarray.Dataset> Dimensions: (dim_0: 1) Dimensions without coordinates: dim_0 Data variables: a (dim_0) int64 0 b (dim_0) int64 1 which it shouldn't do, it should have just immediately failed because there are no dimension coordinates, and otherwise single-element arrays should obviously behave the same as the other examples above. I think it's because it's reading the single element data 0 or 1 as a boolean at some point...

If we gave all the DataArray objects the same name when converted into Dataset objects, then I think the result could always be a single DataArray?

I suppose so, but this seems like an odd way to handle it to me. You're throwing away data (the names) which in other circumstances would be used.

This would be consistent with how we do arithmetic with a DataArrays: the names get ignored, and then we assign a name to the result only if there are no conflicting names on the inputs.

Do we want consistency with arithmetic, or consistency with merge? I strongly feel it should be the latter, as combine wraps merge (In fact combine_nested(dataarrays, concat_dim=None) == merge(dataarrays)). More generally, I think we should try to make the behaviour of all our "combining" functions (i.e. merge, concat, update, combine_nested, and combine_by_coords) be name-aware.

Let me try to clarify by summarizing. Currently, merge and combine_nested both do the same thing for named DataArrays (return a Dataset) and un-named DataArrays (throw an error). python da1 = xr.DataArray(name='foo', data=np.random.rand(3,3), coords=[('x', [1, 2, 3]), ('y', [1, 2, 3])]) da2 = xr.DataArray(name='foo2', data=np.random.rand(3,3), coords=[('x', [5, 6, 7]), ('y', [5, 6, 7])]) merge([da1, da2]) and python da1 = xr.DataArray(name='foo', data=np.random.rand(3,3), coords=[('x', [1, 2, 3]), ('y', [1, 2, 3])]) da2 = xr.DataArray(name='foo2', data=np.random.rand(3,3), coords=[('x', [5, 6, 7]), ('y', [5, 6, 7])]) xr.combine_nested([da1, da2], concat_dim=None) both return <xarray.Dataset> Dimensions: (x: 6, y: 6) Coordinates: * x (x) int64 1 2 3 5 6 7 * y (y) int64 1 2 3 5 6 7 Data variables: foo (x, y) float64 0.5235 0.4114 0.7112 nan nan ... nan nan nan nan nan foo2 (x, y) float64 nan nan nan nan nan ... nan 0.08344 0.8844 0.7462 This all makes intuitive sense to me.

combine_by_coords is basically the same operation as combine, just with automated rather than manual ordering. It will also merge different variables together, so it should do the same thing as merge and combine_nested: fill the gaps in the hypercube up with NaNs (as per #3649) and return a Dataset with two variables, set by the names of the input DataArrays. (and throw an error for un-named DataArrays).

However, as shown above, combine_by_coords is not consistent with merge or combine_nested, which this PR will fix.

This is all different to the arithmetic logic, but I think it makes way more intuitive sense. It's okay for arithmetic and combining logic to be different, as they are used in different contexts and it's an unambiguous delineation to ignore names in arithmetic, and use them in top-level combining functions.

Also, to complete the consistency of the "combining" functions, I think we should make concat name-aware, as described in #3315.

In short: I propose that "combining" isn't arithmetic, and should be treated separately (and consistently across all types of combine functions).

Even for an input of only DataArrays, depending on the actual names and values in the DataArrays, the result of a combine could be a DataArray or a Dataset.

If we gave all the DataArray objects the same name when converted into Dataset objects, then I think the result could always be a single DataArray?

This would be consistent with how we do arithmetic with a DataArrays: the names get ignored, and then we assign a name to the result only if there are no conflicting names on the inputs.

1. Promote all inputs to Datasets (or @friedrichknuth's "dict_like_objects")
2. Do the combining

I agree.

1. If the result has only a single variable then demote from Dataset to DataArray?

I think the simplest solution is to always return Dataset? Then behaviour is always predictable. Otherwise you could combine a bunch of Datasets with a single variable and end up with a DataArray. Alternatively, return a DataArray only when all inputs are DataArrays with the same name.

would it make sense for the combine methods to always return an object of the same type as the inputs? E.g., list of DataArray -> DataArray, list of Dataset -> Dataset?

I don't think so. Even for an input of only DataArrays, depending on the actual names and values in the DataArrays, the result of a combine could be a DataArray or a Dataset. So would it not it be simpler to

1) Promote all inputs to Datasets (or @friedrichknuth's "dict_like_objects") 2) Do the combining 3) If the result has only a single variable then demote from Dataset to DataArray?

That way the result is always the simplest object that can hold the result of combining that particular set of inputs, and the combining internals only have to handle DataSet objects.

EDIT: Oh wait but that won't work in the case of unnamed DataArrays right? EDIT2: Actually _to_temp_dataset will handle that case too?

Thinking about the interface here, would it make sense for the combine methods to always return an object of the same type as the inputs? E.g., list of DataArray -> DataArray, list of Dataset -> Dataset? Effectively, we would disable the automatic merging when working with DataArray objects and only do concat (ignoring DataArray names).

I guess the confusing part is that in concat we always return the same type as the inputs, vs merge where we always return a Dataset. And of course, combine can do both!