Refactor land-ice mesh generation test cases #590
Conversation
trhille
changed the title
build_cell_width() to compass.landice.mesh
trhille
changed the title
build_cell_width() to compass.landice.mesh
trhille
added
land ice
clean-up
in progress
trhille
changed the title
|
To do:
|
TestingI have visually confirmed in Paraview that cell locations are the same when created by this branch and on the current head of
Furthermore, all tests pass on this branch when using the default config files, while some tests fail on |
|
Note: region masks only extend to the ice margin, which will be problematic in the future. This should eventually be dealt with, but is not within the scope of this PR: |
Move build_cell_width() to compass.landice.mesh to greatly reduce code redundancy between all mesh-generating test cases.
Move most jigsaw and mpas-tools operations to compass.landice.mesh.build_MALI_mesh() to greatly reduce redundant code between mesh generation test cases.
Standardize mesh generation test case and config section names, using mesh_gen for the test case and 'mesh' for the config section. All .cfg files pertaining only to mesh generation have been moved into the mesh_gen modules.
Update landice api in developers guide to include renamed methods and classes, and new functions in landice.mesh.
Fix inconsistent usage of Humboldt.nc and Humboldt_1to10km.nc that caused an error at the end of the test case.
Add make_region_masks() function to landice.mesh, and add this capability for Greenland as well as Antarctica.
Fix netcdf format and engine definitions, which for some reason need to be called as mpas_tools.io.default_engine rather than using: from mpas_tools.io import default_engine. The latter approach returned a None type variable, which threw an error when passed to compute_mpas_region_masks.
Fix path to gridded dataset to use for building MALI mesh, in order to properly remove disconnected islands from the mesh.
Add compass.landice.mesh to framework documentation, and update some doc-strings.
trhille
force-pushed
the
landice/mesh_test_cases_refactor
branch
from
5584ea8 to
1817dbb
Compare
Fix a few typos that only became apparent after building docs.
|
@hollyhan , @xylar , and @matthewhoffman, I often feel that requesting three reviews is overkill, but I think it is probably justified in this case, since it is a pretty major refactor. However, if any of you disagree, please let me know! |
|
And @xylar, I don't understand why the |
This should reportedly fix:
```
Error: There was an issue sorting changed files from Github.
Exception: {
"error": "500/TypeError",
"from": "sortChangedFiles",
"message": "There was an issue sorting files changed.",
"payload": "{}"
}
```
See:
trilom/file-changes-action#104
According to trilom/file-changes-action#104 (comment), the |
Thanks for taking care of that! |
There was a problem hiding this comment.
@trhille, this is amazing work! I really love to see redundancy reduced like this. It clearly show the power of compass over its predecessor and should make it a lot easier for future developers to make changes while also hopefully making things easier to maintain.
I do have some suggested changes, all in the realm of cosmetic improvements to the code, formatting changes to the docs, and clarification of the docs and docstrings. So nothing big just a lot of small things.
I didn't do any testing but am happy to if others don't have it covered.
Change function names to all lowercase in compass.landice.mesh.
Remove extraneous information from Users Guide. Add explanation of defining x/y bounds in example config file.
Clean up doc-strings and add a list of required config options for each function in compass/landice/mesh.py that requires section_name as a parameter. Refer to the Users and Developers guides for more information on the meaning of the config options.
Add a few more required config parameters to doc-strings.
Allow the user to define the bounds around a regional domain with any combination of floats and 'None'. For example, a config file that contained ``x_min = 'None'; x_max = 1.0e6; y_min = -1.0e6; y_max = 'None'`` would use you left and top boundaries of the gridded dataset, but also the user-defined values for the right and bottom boundaries.
Remove redundant use of ``flood_mask == 0`` Co-authored-by: Xylar Asay-Davis <xylarstorm@gmail.com>
Remove unnecessary calls to logger.info(), as check_call already takes care of writing the commands to the log file.
|
@xylar, thanks for the rapid turnaround with a very thorough review! I've addressed and responded to each of your requested changes. I've also re-run the mesh generation cases for Kangerlussaq and Koge_Bugt_S to ensure that I didn't break something in the process. |
There was a problem hiding this comment.
@trhille, thanks for making the changes I requested! I'm going to leave testing to you, @matthewhoffman and @hollyhan.
|
Hi @trhille, I tested the I'm happy to test it again, so let me know :) |
|
@hollyhan , thanks for testing! I recognize this error, but in the past I've seen it when the geojson file is formatted incorrectly, which shouldn't be the case here. I'll look into it further. |
|
@hollyhan, I'm unable to reproduce that error on Chicoma using |
|
I was able to successfully run the koge_bugt_s/mesh_gen case on Chicoma using the branch as is, and creating a new compass env through the standard procedure. |
|
@matthewhoffman, thanks! I also tried rebasing onto the current head of |
|
|
||
| Parameters | ||
| ---------- | ||
| section_name : str |
There was a problem hiding this comment.
@trhille , I'm curious about the choice of having this as an argument. It seems in all the test cases using this function, they all use the section mesh. Can you imagine a use case where we wouldn't want to use that section name? If build_cell_width always used the mesh section, this argument could be removed. (And same for build_mali_mesh.)
There was a problem hiding this comment.
Yeah, I thought about hard-coding it too. But I can imagine a case in which we want to produce multiple meshes by running a single test case, for example in a mesh convergence study, in which case we might have multiple sections called "mesh_1km", "mesh_2km", "mesh_4km" or similar. So, even though that case would require a bit of extra editing of that particular case's mesh.py, I think leaving it like this allows for more flexibility with minimal required changes in the future.
There was a problem hiding this comment.
Wouldn't it be better, then, to not have the framework take config options but instead take parameters? That way, the parameters could be computed algorithmically based on a more limited number of config options. Having a bunch of kind of redundant config sections isn't really consistent with the philosophy of compass.
There was a problem hiding this comment.
Wouldn't that just be trading redundancy in the config file for redundancy in the code?
There was a problem hiding this comment.
No, you would have config options that are independent of resolution and a bit of code that can compute the associated resolution-dependent parameters. This is what we do in the cosine_bell test case, which lets a user supply a list of resolutions they would like to test:
https://github.com/MPAS-Dev/compass/blob/main/compass/ocean/tests/global_convergence/cosine_bell/cosine_bell.cfg#L26-L33
https://github.com/MPAS-Dev/compass/blob/main/compass/ocean/tests/global_convergence/cosine_bell/forward.py#L89-L112
The implementation is even a little cleaner in polaris.
There was a problem hiding this comment.
Same kind of idea in polaris' version of the baroclinic channel:
https://github.com/E3SM-Project/polaris/blob/main/polaris/ocean/tests/baroclinic_channel/baroclinic_channel.cfg#L25-L39
https://github.com/E3SM-Project/polaris/blob/main/polaris/ocean/tests/baroclinic_channel/baroclinic_channel_test_case.py#L58-L64
https://github.com/E3SM-Project/polaris/blob/main/polaris/ocean/tests/baroclinic_channel/forward.py#L138-L154
There was a problem hiding this comment.
But I don't think you need to change anything in this PR for now, it's just if you find you want to make meshes at a bunch of resolutions from one test case, we should talk about whether there are ways to not hard-code specific resolutions but rather to support any resolution in a reasonable range. We've found that saves quite a bit of time and adds a lot of flexibility if you decide you want a new resolution (or range of resolutions).
There was a problem hiding this comment.
After talking with both of you, my preference would be to leave this as it is for now. The amount of work to modify it seems not worth the trouble for a slight preference. If/when we implement convergence tests we may want to make other adjustments, so it would be more efficient to wait to make any adjustments until then.
There was a problem hiding this comment.
Yes, that was my suggestion as well.
There was a problem hiding this comment.
@trhille , thanks for all the work on simplifying the mesh gen code. I recognize this was not a trivial amount of work, but I expect the reduction in complexity will pay off in the long run.
Move build_cell_width() to compass.landice.mesh and create new build_MALI_mesh() method to greatly reduce code redundancy between all mesh-generating test cases.
Also add make_region_masks() to compass.landice.mesh and add region masks for Greenland.
Checklist
api.rst) has any new or modified class, method and/or functions listedTestingin this PR) any testing that was used to verify the changes