mesh_2d_edge_x and mesh_2d_edge_y to writer #645
Conversation
|
|
@DanielTollenaar Is this still relevant? If so, why is the PR marked as a draft? |
|
@veenstrajelmer, For DHYDRO users I do think it is relevant for the reason mentioned in this draft (2D meshes cannot be read by MDAL due to incompleteness). It's draft as I saw other imperfections (e.g. projection-info) I could add. It's left as I assumed this repo isn't actively maintained by it's owners. As a user it doesn't make much sense to contribute if PR's aren't reviewed nor merged. |
|
Ok, good to know. The network part of the code is being refactored in several steps. Metadata like crs is also valuable indeed, but this also requires an awareness of crs in hydrolib-core. In the future, we will be looking into external packages like xugrid or ugridpy to outsource the netcdf writing toward. However, I think for the short term this PR is already a good addition and we will consider merging it today. |
There was a problem hiding this comment.
@DanielTollenaar the checks are failing, but we cannot rerun them on your fork-PR somehow. Also, we cannot commit to it, this would also kick off the checks again. A simple solution is to copy your edits to a new PR, but we would like to leave the credits with you. Also I have one request to minimize the diff. Would you have time to do this? Preferably today, but I can imagine that is a bit short notice. This is because we would like to release all recent improvements asap and this one is easy to include.
Agreed; refactoring seems absolutely necessary and I prefer the setup of XUGRID https://deltares.github.io/xugrid/ as it's more intuitive (build on xarray). It may be good to have a look at this issue: Deltares/MeshKernel#326 and work-around it in the UGridWriter as long as it isn't fixed in MeshKernel. Now writing will cause a crash if you manipulate the mesh before. |
|
Thanks for pointing to Deltares/MeshKernel#326, I was unaware of it since I only monitor the meshkernelpy issues every now and then, the python API of meshkernel that you are using there. I can understand the difference is a bit arbitrary for externals. Either way, I commented the current status there and linked some related issues. This is indeed important to fix, but that will take a bit more time. |
There was a problem hiding this comment.
Test is still failing. Could you update your branch with main? I expect that your changes should not cause the fm_container run to fail, but I also do not see the ci run being kicked off, so something seems broken. If updating your branch to main does not work, we will merge the changes in a parallel branch.
Done, but the reason for failing suggests something with a missing username and password, doubt merging from upstream will fix that. Feel free to just copy-paste the code in a new branch in your repo. Don't care about credits ;-):
|
|
Yes, I noticed that also, but wanted to make sure it was not because of the branch being outdated. I think we have to look into PR's from forks in the future, since this seems to be the issue. All tests (both ci and fm_container) are green on other recent branches: https://github.com/Deltares/HYDROLIB-core/actions. We will discuss how to approach this. Thanks for the contribution either way! |
Improving
UgridWriterand other to be able to read net-files as UGRID usingMDAL(e.g. within QGIS).Added:
UgridWriterby addingmesh2d_edge_xandmesh2d_edge_yreading it fromnetwork._mesh2d(meshkernel.Mesh2d)