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

Mesh model #412

Merged
merged 17 commits into from
Aug 21, 2023
Merged

Mesh model #412

merged 17 commits into from
Aug 21, 2023

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Jun 19, 2023

Issue addressed

Fixes #326

Explanation

Supports that self.mesh contains several topologies by implementing changes discussed in #326

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed

@hboisgon hboisgon marked this pull request as ready for review July 20, 2023 09:14
@hboisgon hboisgon changed the title Draft: Mesh model Mesh model Jul 20, 2023
@hboisgon hboisgon mentioned this pull request Jul 24, 2023
11 tasks
@xldeltares
Copy link
Contributor

Hi @hboisgon, I committed the updates from our discussion. Please have a look and make adaptations where needed.

I believe now the review goes back to you, which might be because of my commits...

@xldeltares
Copy link
Contributor

xldeltares commented Aug 7, 2023

HI @hboisgon , I have finished review by testing building and updating mesh2d. This time I tried the make suggestions for review, hoping that it is a bit more clear for communication (also for my remembering where and why I changed something). Could you please review the review (..:D)?

attention points:

  • updated mesh_gdf to write only the geometry of the mesh (without any data, therefore not using to_geodataframe anymore)
  • updated get_mesh and hence mesh_datasets to return UgridDataSet always (mimicking self.mesh)
  • adding optional_attributes = True where needed, mainly when converting grid topology to grid dataset or when transferring information between dataset (context: [help wanted] understand the optional_attributes  xugrid#140)
  • finally, xugrid will have uds.ugrid.names and uds.ugrid.topology available next release (context:support get mesh/grid names  xugrid#141)

Thanks and feel free to book a meeting in my agenda for discussion!

@hboisgon
Copy link
Contributor Author

hboisgon commented Aug 8, 2023

HI @hboisgon , I have finished review by testing building and updating mesh2d. This time I tried the make suggestions for review, hoping that it is a bit more clear for communication (also for my remembering where and why I changed something). Could you please review the review (..:D)?

attention points:

  • updated mesh_gdf to write only the geometry of the mesh (without any data, therefore not using to_geodataframe anymore)
  • updated get_mesh and hence mesh_datasets to return UgridDataSet always (mimicking self.mesh)
  • adding optional_attributes = True where needed, mainly when converting grid topology to grid dataset or when transferring information between dataset (context: [help wanted] understand the optional_attributes  xugrid#140)
  • finally, xugrid will have uds.ugrid.names and uds.ugrid.topology available next release (context:support get mesh/grid names  xugrid#141)

Thanks and feel free to book a meeting in my agenda for discussion!

For optional_attributes discussion, I checked the reply in the linked issue and from this I think it's important we keep during data processing within hydromt but then maybe we make it optional to actually write these to netcdf in the end? Then if some software require them or should not have them, they can decide to write them out or not.

For the new attributes from xugrid on ugrid.names and ugrid.topology, I think for now in core we don't need to update per say as the implementation is quite simple and the interesting part here is to deal with situations if self.mesh is None. But these attributes will be really handy in workflow or external functions dealing with mesh (quite a lot in delft3d-fm we could update and I guess when we do more generic data processing on mesh in the future in hydromt core) :)

@hboisgon
Copy link
Contributor Author

hboisgon commented Aug 8, 2023

Hi @xldeltares , thanks a lot for the very detailed and useful review :) I think we are there now to finally merge this in core. Could you do a final check? Then I will ask someone else from core to also have a look before merging. Many thanks!

Copy link
Contributor

@xldeltares xldeltares left a comment

Choose a reason for hiding this comment

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

Hi @hboisgon , I have reviewed the changes and they look good. hence approving the pull request for the next step in the PR pipeline.

Copy link
Contributor

@savente93 savente93 left a comment

Choose a reason for hiding this comment

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

Looking good, a few small comments, but nothing blocking. Well done!

hydromt/workflows/basin_mask.py Outdated Show resolved Hide resolved
hydromt/models/model_mesh.py Show resolved Hide resolved
hydromt/models/model_mesh.py Show resolved Hide resolved
hydromt/workflows/mesh.py Show resolved Hide resolved
@savente93 savente93 merged commit 1ebbcfb into main Aug 21, 2023
6 of 8 checks passed
@savente93 savente93 deleted the mesh_model branch August 21, 2023 08:04
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.

ENH: update MeshModel.mesh property based on recent xugrid developements
3 participants