Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
JoerivanEngelen opened this issue Nov 24, 2023 · 2 comments

Comments

@JoerivanEngelen
Copy link
Contributor

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
Copy link
Collaborator

Huite commented Nov 24, 2023

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
Copy link
Contributor Author

JoerivanEngelen commented Dec 21, 2023

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?

github-merge-queue bot pushed a commit to Deltares/imod-python that referenced this issue Jan 30, 2024
Fix #708

The changeset is incomplete, as it is not rolled out for all packages
yet. Therefore tests will fail, hence the draft status. This to make the
review process more focused, reviewers are asked to provide feedback on
the approach.

Changes:
- Variable names are inferred based on what is provided to locals().
This makes it possible to avoid having to manually assign variables in
the __init__ of each specific package.
- I had to add one function to work around this issue:
Deltares/xugrid#179
- Grids are merged with an exact join, this to avoid that xarray joins
variable coordinates in an unwanted way when dataarrays are
inconsistent, which causes issues like
#674
- Refactored the riv unit tests to use pytest cases, so that tests
(without the xr.merge) are run for both unstructured and structured
data.

Update:
- Variable names are not inferred with locals() anymore, but instead
with a ``pkg_init`` decorator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants