Add dfmt.meshkernel_delete_withshp() function
#548
Conversation
There was a problem hiding this comment.
Hi @rqwang, I noticed your fork when I was coincidentally browsing trough the dfm_tools forks. Your commits can be a nice contribution. If you clean it up a bit by following my review comments, I would like to merge it.
README.md
Outdated
| @@ -20,3 +20,6 @@ A Python package for pre- and postprocessing D-FlowFM model input and output fil | |||
| - use python>=3.9 to ensure future updates: https://github.com/Deltares/dfm_tools/issues/267 | |||
| - [online documentation](https://deltares.github.io/dfm_tools) with installation guide, contributing guide, tutorials/examples, API reference and a convenient search box. | |||
| - Bug or feature request? Create a [GitHub issue](https://github.com/Deltares/dfm_tools/issues) | |||
| - This fork has a new feature: delete the mesh with shapefiles. Use it in this way: | |||
There was a problem hiding this comment.
remove this, but add a line to docs/whats-new.md
dfm_tools/meshkernel_helpers.py
Outdated
| if coastlines_shp == None: | ||
| print("Need a shape file.") |
There was a problem hiding this comment.
This is not a situation that is expected, since the coastlines_shp argument is not optional. So this can be removed.
dfm_tools/meshkernel_helpers.py
Outdated
| for coastline_geom in coastlines_gdb['geometry']: | ||
| xx, yy = coastline_geom.exterior.coords.xy | ||
| xx = np.array(xx) | ||
| yy = np.array(yy) | ||
|
|
||
| delete_pol_geom = meshkernel.GeometryList(x_coordinates=xx, y_coordinates=yy) #TODO: .copy()/to_numpy() makes the array contiguous in memory, which is necessary for meshkernel.mesh2d_delete() | ||
| mk.mesh2d_delete(geometry_list=delete_pol_geom, | ||
| delete_option=meshkernel.DeleteMeshOption(2), #ALL_COMPLETE_FACES/2: Delete all faces of which the complete face is inside the polygon | ||
| invert_deletion=False) |
There was a problem hiding this comment.
This can be replaced with meshkernel_delete_withgdf() to avoid duplicate code. This would also align it with meshkernel_delete_withcoastlines(). Could you also move your function a bit up in the script, below the similar meshkernel_delete_withcoastlines() function would make more sense.
dfm_tools/meshkernel_helpers.py
Outdated
| None. | ||
|
|
||
| --------------- | ||
| Contributed by Roger Wang (rq.wang@rutgers.edu) |
There was a problem hiding this comment.
It is fine if you like this line here, but credits are also in docs/whats-new.md
dfm_tools/meshkernel_helpers.py
Outdated
| ------- | ||
| None. | ||
|
|
||
| --------------- |
There was a problem hiding this comment.
Remove these dashes, since they might mess up formatting in the docs
dfm_tools/meshkernel_helpers.py
Outdated
| @@ -77,6 +77,43 @@ def meshkernel_delete_withgdf(mk:meshkernel.meshkernel.MeshKernel, coastlines_gd | |||
| delete_option=meshkernel.DeleteMeshOption(2), #ALL_COMPLETE_FACES/2: Delete all faces of which the complete face is inside the polygon | |||
| invert_deletion=False) | |||
|
|
|||
| def meshkernel_delete_withshp(mk:meshkernel.meshkernel.MeshKernel, coastlines_shp, min_area: float=0): | |||
There was a problem hiding this comment.
you could consider adding a crs argument to convert the geodataframe to, in case of models/grids generated in local/cartesian coordinates instead of WGS84. An example can be found in a different function:
dfm_tools/dfm_tools/coastlines.py
Lines 68 to 69 in dda0fb6
There was a problem hiding this comment.
By the way, could you maybe also add a testcase? An example can be found here:
dfm_tools/tests/test_meshkernel_helpers.py
Lines 60 to 72 in dda0fb6
The only thing to add is a shapefile, let me know if this is not feasible and I will add that later.
|
Kudos, SonarCloud Quality Gate passed!
|
|
Thanks for your comments. I'm not sure how to update what's-new.md but I believe all other comments are addressed. Happy to contribute to this great work! |
No description provided.