How should Dataset.update() handle conflicting coordinates?

[shoyer]

Recently, we updated Dataset.__setitem__ to drop conflicting coordinates from DataArray values being assigned if they conflict with existing coordinates (#2087). Because update and __setitem__ share the same code path, this inadvertently updated update as well. Is this something we want?

In v0.10.3, both __setitem__ and update prioritize coordinates from the assigned objects (e.g., value in dataset[key] = value).

In v0.10.4, both __setitem__ and update prioritize coordinates from the original object (e.g., dataset).

I'm not sure this is the right behavior. In particular, in the case of dataset.update(other) where other is also an xarray.Dataset, it seems like coordinates from other should take priority.

Note that one advantage of the current logic (which is violated by my current fix in #2162), is that we maintain the invariant that dataset[key] = value is equivalent to dataset.update({key: value}).

[shoyer]

cc @fujiisoup @crusaderky

[fujiisoup]

I think dataset.update(other) should be equivalent with

for key, value in other.items():
    dataset[key] = value

as similar to python native dict.

Our .item() ony iterates over data_vars not coordinate.
So I think even in dataset.update(other) coordinates from other should be dropped if there is a conflict.

[crusaderky]

I'm of the strong opinion that all joins should be outer joins unless the user explicitly says otherwise, as it's the approach least prone to do damage. I would humbly suggest considering the change for a future major release (0.11 / 0.12), with several minor releases before that printing futurewarnings.

This said, I think that changing from a right join (0.10.3) to a left join (0.10.4) will only cause breakages without providing any actual benefit in terms of user-friendliness, so we should retain the previous behaviour. A right join vaguely makes more sense IMHO as it follows the general phylosophy of dict.update() where rhs wins in case of collision.

[fujiisoup]

I think we should discuss dimension coordinte and non-dimenson coordinate separately.
I guess @shoyer meant non-dimenson coordinate here.

For dimension coordinte, it is always outer join, if I understand correctly.

[crusaderky]

Then I'm not sure I understand the change:

fridge = xarray.Dataset(
    data_vars={
        'fruits': ('fruit', [10]),
    },
    coords={
        'fruit': ('fruit', ['apples']),
        'quality': ('fruit', ['Red Velvet']),
    })
shopping = xarray.Dataset(
    data_vars={
        'fruits': ('fruit', [20]),
    },
    coords={
        'fruit': ('fruit', ['oranges']),
        'quality': ('fruit', ['Tangerine']),
    })
fridge.update(shopping)  # throw away old contents and fill with new stuff

In both 0.10.3 and 0.10.4:

<xarray.Dataset>
Dimensions:  (fruit: 1)
Coordinates:
  * fruit    (fruit) object 'apples'
    quality  (fruit) object nan
Data variables:
    fruits   (fruit) float64 nan

The above doesn't make any sense to me. I wanted to replace the fruits variable with brand new content, and instead I lost both the old and the new?!?

[fujiisoup]

I got cofused... I was wrong. I thought it would be an outer joing

I think we need to discuss two issues here separately.

1. If there are conflicts in dimension coordinate, should it be outer join?
2. If there are no conflicts in dimension coordinate, but there are conflicts in non dimension coordinate, whether left or right should be prioritized?

[fujiisoup]

#2087 changed the second behavior.

In [1]: import xarray 
   ...: 
   ...: fridge = xarray.Dataset(
   ...:     data_vars={
   ...:         'var1': ('fruit', [10]),
   ...:     },
   ...:     coords={
   ...:         'fruit': ('fruit', [1]),
   ...:         'quality': ('fruit', ['Red Velvet']),
   ...:     })
   ...: shopping = xarray.Dataset(
   ...:     data_vars={
   ...:         'var1': ('fruit', [20]),
   ...:     },
   ...:     coords={
   ...:         'fruit': ('fruit', [1]),
   ...:         'quality': ('fruit', ['Tangerine']),
   ...:     })
   ...: 
   ...: fridge['var1'] = shopping['var1']
   ...: 

with v10.3

In [2]: fridge
Out[2]: 
<xarray.Dataset>
Dimensions:  (fruit: 1)
Coordinates:
  * fruit    (fruit) int64 1
    quality  (fruit) <U9 'Tangerine'
Data variables:
    var1     (fruit) int64 20

and with v10.4

In [3]: fridge
Out[3]: 
<xarray.Dataset>
Dimensions:  (fruit: 1)
Coordinates:
  * fruit    (fruit) int64 1
    quality  (fruit) <U10 'Red Velvet'
Data variables:
    var1     (fruit) int64 20

[shoyer]

OK, looking at this more carefully ds.update(other) didn't actually change when other is a Dataset, because ds[k] = ds[k].drop(coord_names) doesn't actually drop coordinates from a Dataset. It just shows a warning now, due to iteration over a Dataset.

So maybe we can leave the current behavior as is for now (but remove the warning).

What did change is how we handle conflicts in __setitem__ (which was intentional), and how we handle conflicts in update when the new value is a dictionary (which was not intentional, but at least remained consistent with __setitem__).

[shoyer]

@crusaderky this behavior you show is indeed really strange. I don't know why alignment of dimensions works that way currently.

[fujiisoup]

So maybe we can leave the current behavior as is for now (but remove the warning).

Agreed.

@shoyer, how do you think about the current __setitem__ behavior with conflict dimension coordinate?
Should it be outer join as @crusaderky pointed out?

EDIT:
I did not noticed the above comment. I will raise an issue for this.

[crusaderky]

If there are conflicts in dimension coordinate, should it be outer join?

Consider this example:

a = Dataset({
    'x': [10, 20],
    'd1': ('x', [100, 200]),
    'd2': ('x', [300, 400])
})
b = Dataset({
    'x': [15],
    'd1': ('x', [500]),
})
a.update(b)

In the above, with anything but an outer join you're destroying d2 - which doesn't even exist in the rhs dataset! A sane, desirable outcome should be

Dataset({
    'x': [10, 20, 15],
    'd1': ('x', [nan, nan, 500]),
    'd2': ('x', [300, 400, nan])
})

If there are no conflicts in dimension coordinate, but there are conflicts in non dimension coordinate, whether left or right should be prioritized?

I think this should be a right join. I alway think of non-index coords as N-to-1 properties of the index. For example,

a = Dataset(
    coords={
        'country': ('country', ['UK', 'France', 'Greece']),
        'currency': ('country', ['GBP', 'EUR', 'EUR']),
    },
    data_vars={
        'GDP': ('country', [1000, 2000, 3000]),
        'Debt': ('country', [100, 200, 300]),
    })
b = Dataset(  # Greece exits the Eurozone
    coords={
        'country': ('country', ['UK', 'France', 'Greece']),
        'currency': ('country', ['GBP', 'EUR', 'GRD']),
    },
    data_vars={
        'GDP': ('country', [1000, 2000, 150000]),
    })
a.update(b)

In the above example, I just broke the Debt variable - as I forgot to perform a currency conversion for the greek debt, which has been silently changed from 300 EUR to 300 GRD.
However I can't see any elegant way to avoid this. I definitely would not like to duplicate the 'country' index.

[fujiisoup]

Thanks, @crusaderky.
The first behavior you pointed out is a bug I think.
I raised an issue in #2184, and maybe it should be discussed there.

For the second example,

I think this should be a right join.

I agree with this.

[fujiisoup]

For referene the original issue in #2068 was

In [4]: ds = xr.Dataset()
   ...: ds.coords['source'] = (['a', 'b', 'c'], np.random.random((2, 3, 4)))
   ...: ds.coords['unrelated'] = (['a', 'c'], np.random.random((2, 4)))
   ...: ds
   ...: 
Out[4]: 
<xarray.Dataset>
Dimensions:    (a: 2, b: 3, c: 4)
Coordinates:
    source     (a, b, c) float64 0.4158 0.07152 0.4258 0.4382 0.6616 0.142 ...
    unrelated  (a, c) float64 0.9318 0.03723 0.4226 0.9472 0.8753 0.7022 ...
Dimensions without coordinates: a, b, c
Data variables:
    *empty*

In [5]: ds['dest-2'] = xr.ones_like(ds['source'].isel(c=0))
   ...: ds
   ...: 
Out[5]: 
<xarray.Dataset>
Dimensions:    (a: 2, b: 3)
Coordinates:
    source     (a, b) float64 0.4158 0.6616 0.1583 0.7821 0.221 0.2555
    unrelated  (a) float64 0.9318 0.8753
Dimensions without coordinates: a, b
Data variables:
    dest-2     (a, b) float64 1.0 1.0 1.0 1.0 1.0 1.0

where ds['unrelated'] drops dimension c.
We changed this behavior in #2087, but I think it was a wrong direction.

The previous behavior might be OK as long as unrelated is a coordinate variable.

EDIT:
I still feel something strange both in the previous and current behavior of __setitem__ with coord.
Generally, as @crusaderky has pointed out, the right join will be a better choice.
But In the above example, dropping the dimension of c of 'unrelated' looks also awkward.