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

fix get_mesh after xugrid update #848

Merged
merged 3 commits into from
Mar 27, 2024
Merged

fix get_mesh after xugrid update #848

merged 3 commits into from
Mar 27, 2024

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Mar 22, 2024

Issue addressed

None defined: MeshModel.get_mesh method returns error after xugrid update to 0.9.0

hydromt/models/components/mesh.py:369: in get_mesh
    uds[var] = self.data[var]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <xarray.Dataset> Size: 386kB
Dimensions:        (mesh2d_nNodes: 2790, mesh2d_nFaces: 5248,
                    mesh2d_...int64 42kB 0 1 2 3 4 ... 5244 5245 5246 5247
Data variables:
    *empty*
Attributes:
    Conventions:  CF-1.9 UGRID-1.0
key = 'elevation'
value = <xarray.DataArray 'elevation' (mesh2d_nFaces: 5248)> Size: 21kB
[5248 values with dtype=float32]
Coordinates:
    mesh...2kB ...
  * mesh2d_nFaces  (mesh2d_nFaces) int64 42kB 0 1 2 3 4 ... 5244 5245 5246 5247
Attributes:
    unit:     m NAP

    def __setitem__(self, key, value):
        # TODO: check with topology
        if isinstance(value, UgridDataArray):
            append = True
            # Check if the dimensions occur in self.
            # if they don't, the grid should be added.
            if self.grids is not None:
                alldims = set(
                    chain.from_iterable([grid.dimensions for grid in self.grids])
                )
                matching_dims = set(value.grid.dimensions).intersection(alldims)
                if matching_dims:
                    append = False
                    # If they do match: the grids should match.
                    grids = {
                        dim: grid for grid in self.grids for dim in grid.dimensions
                    }
                    firstdim = next(iter(matching_dims))
                    grid_to_check = grids[firstdim]
                    if not grid_to_check.equals(value.grid):
>                       raise ValueError(
                            "Grids share dimension names but do not are not identical. "
                            f"Matching dimensions: {matching_dims}"
                        )
E                       ValueError: Grids share dimension names but do not are not identical. Matching dimensions: {'mesh2d_nEdges', 'mesh2d_nNodes', 'mesh2d_nFaces'}

Explanation

Xugrid does not support well having data variables that are not part of the grid definition and their new check grid is too restrictive on optional ugrid dims that are not part of the data variable dims.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed
  • For predefined catalogs: update the catalog version in the file itself, the references in data/predefined_catalogs.yml, and the changelog in data/changelog.rst

@hboisgon
Copy link
Contributor Author

I added a super simple test and also tested with the more complex hydromt_delft3dfm plugin and seems it should now be solved. Can you check and review @Tjalling-dejong ?

Copy link
Contributor

@Tjalling-dejong Tjalling-dejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

@Tjalling-dejong Tjalling-dejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but can you remove the commented code?

hydromt/models/model_mesh.py Outdated Show resolved Hide resolved
hydromt/models/model_mesh.py Outdated Show resolved Hide resolved
@hboisgon
Copy link
Contributor Author

Good one! Was late Friday evening when I finished... Can you review again ?

Copy link
Contributor

@Tjalling-dejong Tjalling-dejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@hboisgon hboisgon merged commit 49f3c07 into main Mar 27, 2024
10 checks passed
@hboisgon hboisgon deleted the bugfix_get_mesh branch March 27, 2024 01:22
@savente93 savente93 mentioned this pull request Jun 14, 2024
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

Successfully merging this pull request may close these issues.

None yet

2 participants