-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add NetCDF C version of mesh conversion tools #514
Add NetCDF C version of mesh conversion tools #514
Conversation
These work well even for very large meshes. The mask creator has not yet been ported and is not included.
@mgduda, I want to make sure you're okay with this addition. I realize it could be disruptive or cause confusion for your group to have Please give me your feedback as soon as you're able because we'd like to get this merged in and into use if you're okay with it. |
@mark-petersen and @matthewhoffman, I'm making sure you're aware of this change, since it could affect MPAS-Ocean and MALI. (I'm not sure who to ping for MPAS-Seaice at this point. @darincomeau, is that you?) |
@dengwirda, I'd like your feedback on whether the version of the code here looks good to you. I made only minor bug fixes, so it should basically be what you gave me. Please also let me know if it works for you. You can install it yourself into a conda environment with:
|
TestingI tested this with the compass I also plan to test this on the RRS6to18 mesh in MPAS-Dev/compass#576, which failed miserably with the existing |
@mgduda, @mark-petersen, @matthewhoffman and @dengwirda, just a reminder that I'm looking for feedback on this PR. |
Thanks @xylar, I've had success on
where
With an 18M cell mesh the output is:
and I suspect the non-vectorised call to build the circumcentres is a place where a lot of the step 1 time is being spent. |
We want to be able to call `mpas_tools.io.logging.check_call()` with `logger=None` so we can handle cases with and without a logger without any special treatment.
cb2f235
to
57d374b
Compare
@mgduda, @mark-petersen, @matthewhoffman, another request for you to have a look at this please. |
The `cullCell` field doesn't necessarily exist and we want to not raise an exception if it doesn't.
For now, we don't have an alternative because the mask creator has not yet been rewritten to support NetCDF-C. This merge also switches all 3 functions in the mesh.creation module to use the logging.check_call() function, rather than an equivalent private function.
The calls to `convert()` and `cull()` wrappers have caused trouble for big meshes.
The `cull()` wrapper is more trouble in this case.
57d374b
to
325d225
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work on this! If the new version passes the PR suite bfb, then it is certainly working. I've looked over the code. I am mostly trying to understand it, and I see that this code will be helpful in learning c++ for Omega. It is strange that the c version of the netcdf libraries are supported and the c++ are not - I would have expected the opposite.
Thanks @mark-petersen! |
There are several things to say about this. First, both the NetCDF Fortran and C++ libraries are based on (wrappers around?) the NetCDF C library. So the C library gets a lot more attention than the others. The C++ library hasn't been updated in years. To add to the confusion, there was a major rewrite of the NetCDF C++ API awhile back, and our tools use the NetCDF C++ library from before the rewrite, so they are using the "legacy" NetCDF C++ library. That has made a bad situation worse. The move to the NetCDF C library in @dengwirda's version of the tools means we're using the library that gets the most maintenance, so I think that's unquestionably the right direction to go. |
@@ -102,7 +102,7 @@ test: | |||
- translate_planar_grid -f 'periodic_mesh_10x20_1km.nc' -d 'periodic_mesh_20x40_1km.nc' | |||
- MpasMeshConverter.x mesh_tools/mesh_conversion_tools/test/mesh.QU.1920km.151026.nc mesh.nc | |||
- MpasCellCuller.x mesh.nc culled_mesh.nc -m mesh_tools/mesh_conversion_tools/test/land_mask_final.nc | |||
- MpasMaskCreator.x mesh.nc arctic_mask.nc -f mesh_tools/mesh_conversion_tools/test/Arctic_Ocean.geojson | |||
# - MpasMaskCreator.x mesh.nc arctic_mask.nc -f mesh_tools/mesh_conversion_tools/test/Arctic_Ocean.geojson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as I said, this tool has been removed from the conda package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked this over to get a sense of the changes, but I don't feel I'm in a position for a critical review. The argument for doing this is convincing, and if the new version works as expected, then I'm happy to approve the PR.
ds_culled = cull(dsIn=ds_mesh, dsInverse=ds_mask, logger=logger, | ||
dir=temp_dir) | ||
write_netcdf(ds_culled, out_mesh_filename) | ||
args = ['MpasCellCuller.x', | ||
mesh_filename, | ||
out_mesh_filename, | ||
'-i', f'{temp_dir}/mask.nc'] | ||
check_call(args=args, logger=logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewhoffman, I saw a question about this change (that maybe got deleted).
In general, I'm moving away from the wrapper function calls in any case where we want to read in and write out files anyway. The wrapper functions have to write out temporary files before calling the tools under the hood, so there's an unnecessary extra level of reading in and writing back out files with the wrapper functions if they already exist.
@mgduda, I'm assuming you may be away. I am going to merge this on Monday unless I hear from you. |
871ec07
to
781726e
Compare
We need one per optional variable or attribute. Otherwise all subsequent variables or attributes get skipped when the first is not present.
781726e
to
19ed985
Compare
Remove limit on number of arguments (since the code handles unlimited numbers of masks)
19ed985
to
3e28c22
Compare
More testingWith my recent fixes to the cell culler, I am re-testing with the following meshes. A check mark means the mesh produced by compass is BFB the same as with 0.21.0 release.
|
This merge adds a new version of the mesh conversion tools that uses the standard NetCDF C library, rather than the legacy NetCDF C++ library.
Most of the work was done by @dengwirda, so I can only take credit for the conda-package plumbing and some minor debugging and clean-up.
We have encountered significant problems with the legacy NetCDF C++ library. Foremost is that it does not perform well on large meshes (over several million cells). The library is not being developed further and even the successor NetCDF C++ library has not had a release in about 4 years and does not appear to be in active use by many projects.
I have chosen to leave the existing mesh creation tools unchanged for now. The new tools are in their own
mesh_creation_tools_netcdf_c
directory. We could eventually decide to removemesh_creation_tools
and adoptmesh_creation_tools_netcdf_c
in their place but I have not done that so far.@dengwirda has ported the mesh converter and cell culler but not the mask creator. This could be done by anyone interested but our group has other tools we use for mask creation that we use instead, so it is not a priority for us. This is one of the reasons for maintaining
mesh_creation_tools
as it is.I have changed the conda package to use
mesh_creation_tools_netcdf_c
. Since the mask creator is not available, I have change thempas_tools.mesh.conversion.mask()
method to use the python masking capabilities that are part of the conda package.Finally, this merge includes changes to how
mpas_toolsmesh.creation.build_mesh()
works: It now usessubprocess
calls rather than theconvert()
andcull()
wrapper functions, since the wrapper functions have caused out-of-memory errors for large meshes (and provide no added value in this situation). Note: if desired, I could break this change into its own PR.