Deltares / xugrid

Xarray and unstructured grids
https://deltares.github.io/xugrid/
MIT License
58 stars 8 forks source link

xugrid.merge does not support merging list of dicts wheares xarray.merge does #179

Open JoerivanEngelen opened 9 months ago

JoerivanEngelen commented 9 months ago

A feature I often use in xarray to construct a dataset is by merging a dictionary of DataArrays. (Note that the dictionary needs to be in a list, to allow multiple dictionaries to be merged as well)

MWE:

import xarray as xr
da_dict = {"a": xr.DataArray(), "b": xr.DataArray()}
xr.merge([da_dict])

In xugrid, however, this fails in the type checking:

import xugrid as xu
uda = xu.data.elevation_nl()
uda_dict = {"a": uda, "b": uda}
xu.merge([uda_dict])

Throws:

TypeError: Can only concatenate xugrid UgridDataset and UgridDataArray objects, got dict

If it is difficult to support this (the generic typechecker for wrapped xarray functions seems quite neat), it would be good to either document that this goes unsupported somewhere and to document some workarounds.

Huite commented 9 months ago

Is it really that hard to support this?

How does xarray do the merging here? I guess it just dispatches on type as well?

We can special case the merge implementation (and let it call the wrapped method internally). I think the biggest risk is having it misfire and give confusing errors or something.

You wouldn't want to change the wrapping function, because the types only make sense for the merge.

JoerivanEngelen commented 8 months ago

How does xarray do the merging here? I guess it just dispatches on type as well?

It appears xarray forces everything to dict-like objects https://github.com/pydata/xarray/blob/main/xarray/core/merge.py#L994 (So either datasets or dicts), then calls merge_core with this.

Btw, until this is resolved, the workaround for any user running into this issue is to do:

import xugrid as xu
uda = xu.data.elevation_nl()
uds_ls = [uda.to_dataset(name = "a"), uda.to_dataset(name = "b")]
uds = xu.merge(uds_ls)

You wouldn't want to change the wrapping function, because the types only make sense for the merge.

Relevant function

This only appears to be used to wrap xr.concat and xr.merge. So might be possible?