Operations on DataTree objects should not create duplicate coordinates on sub-trees

[shoyer]

What is your issue?

This is very obviously an issue using the repr from #9470:

In [20]: tree = DataTree(Dataset(coords={'x': [1, 2, 3]}))

In [21]: tree['/foo'] = DataTree(Dataset({'bar': ('x', [4, 5, 6])}))

In [22]: tree
Out[22]:
<xarray.DataTree>
Group: /
│   Dimensions:  (x: 3)
│   Coordinates:
│     * x        (x) int64 24B 1 2 3
└── Group: /foo
        Dimensions:  (x: 3)
        Data variables:
            bar      (x) int64 24B 4 5 6

In [23]: tree * 2
Out[23]:
<xarray.DataTree>
Group: /
│   Dimensions:  (x: 3)
│   Coordinates:
│     * x        (x) int64 24B 1 2 3
└── Group: /foo
        Dimensions:  (x: 3)
        Coordinates:
          * x        (x) int64 24B 1 2 3
        Data variables:
            bar      (x) int64 24B 8 10 12

Instead, the result for tree * 2 should look like:

<xarray.DataTree>
Group: /
│   Dimensions:  (x: 3)
│   Coordinates:
│     * x        (x) int64 24B 1 2 3
└── Group: /foo
        Dimensions:  (x: 3)
        Data variables:
            bar      (x) int64 24B 8 10 12

We probably also need to also revisit other Dataset methods that are ported to DataTree via map_over_subtree (xref #9472). Some of these (e.g., arithmetic, aggregations) can likely easily be corrected simply by mapping over nodes with inherited=False. Others (e.g., indexing) will need more careful consideration.

[shoyer]

As a starting point, it might make sense to disable most automatically ported DataTree methods, at least those that can't be implemented via map_over_subtree with inherited=False.

[TomNicholas]

I'm kind of surprised that the existing map_over_subtree tests didn't catch this.

[shoyer]

I'm kind of surprised that the existing map_over_subtree tests didn't catch this.

This could be because assert_identical (prior to #9473) doesn't currently check this.

[TomNicholas]

In the meeting just now we decided that this is a deficiency in the data model, not just in the implementation of map_over_subtree. We should change the model so that identical coordinates in children are automatically removed, so that the above would be automatically de-duplicated at construction time.

This is important to do as otherwise almost every user will run into this problem, especially using this pattern:

dt['lat'] = ...  # coord present on root

ds = dt[path].dataset
result_ds = some_operation(ds)
dt[path].dataset = result_ds  # coord now present on both root and descendant!

Currently this would duplicate the 'lat' coordinate between dt and dt[path], which would lead to very confusing behaviour, e.g.

del dt[path]['lat']
'lat' in dt[path]  # would return true!

(because only the duplicated coordinate on the child would have been deleted, not the original on the root, which would still be inherited).

IO

Disallowing duplicated coordinates in general would subtly affect round-tripping behaviour. It would no longer be possible to write out a netCDF/Zarr file with duplicated coordinates using DataTree, because DataTree could not represent them in the first place.

Round-tripping is then broken for any file with duplicated coordinates, as reading the file would de-duplicate the coordinates, and the information wouldn't be propagated in order to re-duplicate them before writing.

Mostly this wouldn't matter often - there is not much reason to define duplicated coordinates in netCDF/Zarr in the first place.

It might lead to subtle trickiness for files containing CF metadata that refers to other variables in different groups using absolute paths, but that is not something that xarray promises to handle anyway, as interpreting metadata contents is not part of xarray's data model (beyond what happens during decoding).

However again open_groups could still open such files without removing duplicates, and one could imagine writing out group-by-group to a single output file, and that should roundtrip perfectly.

All of this is a consequence of the fact that whilst very similar, the data models of netCDF, Zarr, and DataTree are not identical.

cc @owenlittlejohns @flamingbear @eni-awowale

---

Not sure exactly what the internal implementation of de-deduplication would be (perhaps some kind of NonDuplicatedChainMap?), but @shoyer said he is on it 🙂 A small change to map_over_subtree might also still be required to close this issue.

[shoyer]

It should be straightforward enough to handle duplicated index coordinates. Non-identical coordinates with an index are already disallowed between parent and child nodes, because the parent and child nodes would fail the alignment check in _pre_attach.

However, there is one tricky case: duplicated coordinates without an associated index. Here's an example of how these currently manifest themselves:

In [33]: tree = DataTree(Dataset(coords={'x': 0}), children={'child': DataTree(Dataset({'y': 1}))})

In [34]: tree
Out[34]:
<xarray.DataTree>
Group: /
│   Dimensions:  ()
│   Coordinates:
│       x        int64 8B 0
└── Group: /child
        Dimensions:  ()
        Data variables:
            y        int64 8B 1

In [35]: tree.map_over_subtree(lambda ds: ds)
Out[35]:
<xarray.DataTree>
Group: /
│   Dimensions:  ()
│   Coordinates:
│       x        int64 8B 0
└── Group: /child
        Dimensions:  ()
        Coordinates:
            x        int64 8B 0
        Data variables:
            y        int64 8B 1

Notice the duplicated coordinates /x and /child/x.

For consistency with the rest of Xarray, we should try to handle this like duplicate coordinates that result from other Xarray operations like arithmetic. As noted in #9481, the current strategy requires evaluating coordinate values to see if they are equal or not in order to decide where they appear. This is attractive for DataTree as well, but unfortunately is not compatible with lazy evaluation.

Instead, I propose that we switch to the equivalent of compat='override, similar to the proposal for Dataset/DataArray coordinates proposed in #9481. On DataTree, if an operation results in a node with a (non-index) coordinate with the same name as a parent coordinate, the child coordinate should be silently dropped.

An exception to this rule are operations that explicitly assign a child coordinate, e.g., DataTree.coords.__setitem__. Here we should raise an error (noting the existence of the parent coordinate) rather than being a silent no-op.

[eni-awowale]

Thanks for this great write up! I think in the long run this will probably help to avoid confusion and is probably the best way forward. But from more of a DAAC archiving, service provider and data validation perspective (lol I wear a lot of hats) this makes me a little nervous. At GES DISC we use DataTree to do data validation checks against server hosted services and cloud hosted services. The part that makes me nervous is that using DataTree to open netCDF4 files would be modifying the orginal file by removing the duplicate coordinate variables at each node. I don't think this will actually break anything in the backend for us but there is some complexity especialy with metadata that was mentioned.

Would folks be opposed to some kind of flag that's like inherit=True or de_duplicate=True? I think we could set that to being the default behavior and give the user an option to turn it off.

[TomNicholas]

Would folks be opposed to some kind of flag that's like inherit=True or de_duplicate=True? I think we could set that to being the default behavior and give the user an option to turn it off.

The proposed change here would make such a flag impossible - your intended result of de_duplicate=False would now be forbidden by the DataTree data model.

What we could perhaps do though is add a flag to constructors / openers that would raise on encountering duplicated coordinates instead of silently de-duplicating them. e.g.

dt = open_datatree(unvalidated.nc, duplicate_coords='raise')

where the error message tells you which coordinates are duplicated, and refers you to open_groups. The default could still be 'ignore' or maybe 'warn'. There's precedent for treating potential errors this way, see for example

xarray/xarray/namedarray/core.py

Line 1001 in aeaa082

missing_dims: ErrorOptionsWithWarn = "raise",