Add calving law tests for Humboldt test group #318
Conversation
|
Hello @matthewhoffman! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-04-27 18:48:40 UTC |
matthewhoffman
added
in progress
matthewhoffman
force-pushed
the
landice/add_calving_law_tests
branch
2 times, most recently
from
992d40f to
015cefc
Compare
|
@matthewhoffman, whenever it's convenient (perhaps when you're doing the PEP8 fixes), could you rebase onto |
|
Sure, I will do that. You get an email every time any PRs fail CI? Is that only triggered when a new PR is opened or an existing PR is modified (e.g. new commits are pushed)? Or does it happen nightly on open PRs? |
|
I get an email every time you push a new commit to an open PR and CI fails. So that's a lot of emails. They're easy to throw away but there's a chance something actually important gets lost. |
matthewhoffman
force-pushed
the
landice/add_calving_law_tests
branch
from
015cefc to
c555b22
Compare
|
@matthewhoffman, note that the eigencalving tests fail on execution due to the dreaded >10% failed ablation error. I reduced |
matthewhoffman
force-pushed
the
landice/add_calving_law_tests
branch
from
c555b22 to
e925d49
Compare
|
I fixed PEP8 issues and rebased this branch so the spack build of Albany within COMPASS is available. I retested the two test suites in this PR and got these results:
Note that the goal for this PR is not to have all tests pass, but to provide tests for each configuration.
|
|
The runs with FO solver on this 1km Humboldt mesh are (not surprisingly) very slow. For the sake of the calving tests, the 3km Humboldt mesh would probably be sufficient and would be much faster. @trhille , do you think that makes sense? If so, can you provide a 3km Humboldt mesh to put on Anvil? (and let me know the yaml settings to use). I can update the test suite accordingly. |
Yeah that's a good idea. The 2–8 km mesh might also be a good choice. |
|
After switching to 3km mesh, same tests pass and fail, but suite runs twice as fast: |
|
I adjusted the tolerances for the FO decomp tests to account for non-BFB decomp behavior of the FO solver. After that I get the following results for the humboldt_calving_tests_FO suite: The one failure is a known issue that will be resolved later in MALI. |
matthewhoffman
removed
the
in progress
|
This PR is now complete and ready for review. I suggest running the
I also tested the |
|
My testing of the |
|
Great, I'll test tomorrow. |
There was a problem hiding this comment.
@matthewhoffman, terrific work on this! It will be a huge help for future development. I have a handful on minor comments.
|
|
||
| # Set up tests without calving using the 1km mesh | ||
| mesh_type = '1km' | ||
| for velo_solver in ['FO', ]: |
There was a problem hiding this comment.
Do you want a 'none' option in this loop as well?
There was a problem hiding this comment.
Presumably, the 1km mesh is purposefully being run only with the FO solver.
BTW, the syntax you have isn't wrong but it's unusual. This would be more typical:
| for velo_solver in ['FO', ]: | |
| for velo_solver in ['FO']: |
There was a problem hiding this comment.
@trhille , I was trying to keep the number of new tests added by this PR to a minimum, so I deliberately only included the FO solver for the 1km mesh, since none of the 1km tests are in any suites. My thought was it would be nice to be able to set up a high res Humboldt test with COMPASS in a standardized way. I'm open to adding other versions of this test if you think they'd be useful.
|
|
||
| # Discretization Description | ||
| Discretization: | ||
| Exodus Output File Name: albany_output.exo |
There was a problem hiding this comment.
This is fine, but could also comment out if we don't need all these exodus files.
There was a problem hiding this comment.
I commented that line in e8ef60e.
| @@ -0,0 +1,183 @@ | |||
| from compass.validate import compare_variables | |||
| from compass.testcase import TestCase | |||
| # from compass.landice.tests.humboldt.mesh import Mesh | |||
There was a problem hiding this comment.
Removed in 264047c
| """ | ||
| A test case for performing two MALI runs of a humboldt setup, one with one | ||
| core and one with four. The test case verifies that the results of the | ||
| two runs are identical or close to identical. |
There was a problem hiding this comment.
Add a note explaining why "close to identical" is necessary.
|
|
||
| # Commented code to make use of mesh generation step | ||
| # Note it will not include uReconstructX/Y or muFriction! | ||
| # It will also add a few minutes run time to the test! |
There was a problem hiding this comment.
Would we ever want to do this?
There was a problem hiding this comment.
The way we handle this on the ocean side is that we have the test case configured to use a mesh step to create the mesh but we have that test run with "cached output" in the test suite, see various uses of cached here:
https://github.com/MPAS-Dev/compass/blob/master/compass/ocean/suites/pr.txt
The cached data gets stored on the LCRC server in a specific "database" that mimics the work directory:
https://web.lcrc.anl.gov/public/e3sm/mpas_standalonedata/mpas-ocean/compass_cache/
I think this would be worth considering for a follow-up PR here.
There was a problem hiding this comment.
@trhille , good point. Given we don't expect the basal friction optimization to be automated any time soon, we probably will never completely set up the mesh this way. I could imagine interpolating basal friction from an existing file just to be able to be able to test and end to end workflow, but that might be more trouble than it's worth. I'll remove these comments. If we ever want to do that, we can figure it out easily enough.
There was a problem hiding this comment.
Removed in 264047c
| variables = ['thickness', 'surfaceSpeed'] | ||
|
|
||
| if self.calving_law is not None and self.calving_law != 'none': | ||
| variables.append('calvingThickness') |
There was a problem hiding this comment.
Just a thought: Are there situations in which we would want to compare calvingVelocity as well as calvingThickness? If calvingThickness is BFB, then we know that calvingVelocity is, but if calvingThickness is not BFB, it's still possible that calvingVelocity is BFB and the issue is elsewhere.
There was a problem hiding this comment.
That's a good suggestion. We know most of the trouble is likely to come in the conversion of calvingVelocity to calvingThickness, so it is likely to be useful to check both fields. Given these tests are already pretty "heavy" I don't have a problem with including an additional field verification.
There was a problem hiding this comment.
I added calvingVelocity in 74a03d3. This already came in handy for debugging the ismip6-retreat issues I worked on today.
|
|
||
| <stream name="output" | ||
| output_interval="0000-00-01_00:00:00" | ||
| reference_time="0000-01-01_00:00:00" |
There was a problem hiding this comment.
Does it matter that this reference_time is different from the one above?
There was a problem hiding this comment.
Yeah, that's confusing that should be fixed.
There was a problem hiding this comment.
Fixed in f45527f. I also changed start dates and references dates to January 1. That exposed some new failures that I've fixed in MALI-Dev/E3SM#32 (but will appear as failures when running the tests in this PR with MALI-Dev/develop). So it was useful to take a closer look at these settings.
| ------------------ | ||
| The compass.landice.tests.humboldt.decomposition_test.DecompositionTest | ||
| performs the same simulation on different numbers of cores. It ensures | ||
| relevant variablesd are identical or have expected differences. |
| restart_test | ||
| ------------ | ||
| The compass.landice.tests.humboldt.restart_test.RestartTest performs a | ||
| run of a full specified duration followe by a short run plus a restart |
There was a problem hiding this comment.
I'm seeing 3 test failures for each suite on Anvil with mvapich:
00:06 FAIL landice_humboldt_mesh-3km_decomposition_test_velo-none_calving-von_mises_stress
00:05 FAIL landice_humboldt_mesh-3km_decomposition_test_velo-none_calving-ismip6_retreat
00:05 FAIL landice_humboldt_mesh-3km_decomposition_test_velo-none_calving-von_mises_stress_damage-threshold_faceMelting
...
01:07 FAIL landice_humboldt_mesh-3km_decomposition_test_velo-fo_calving-von_mises_stress
01:19 FAIL landice_humboldt_mesh-3km_decomposition_test_velo-fo_calving-ismip6_retreat
01:01 FAIL landice_humboldt_mesh-3km_decomposition_test_velo-fo_calving-von_mises_stress_damage-threshold_faceMelting
Results are in:
/lcrc/group/e3sm/ac.xylar/compass_1.0/anvil/test_20220422/mali_calving
/lcrc/group/e3sm/ac.xylar/compass_1.0/anvil/test_20220422/mali_calving_fo
if you want to investigate.
| @@ -0,0 +1,18 @@ | |||
| landice/humboldt/mesh-3km_decomposition_test/velo-fo_calving-none | |||
There was a problem hiding this comment.
Please consider changing the FO in this suite name to fo to be consistent with other suite names (all lowercase).
There was a problem hiding this comment.
Changed in 7615549
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-none | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-floating | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-eigencalving | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-specified_calving_velocity | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-von_mises_stress | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-damagecalving | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-ismip6_retreat | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-von_mises_stress_damage-threshold_faceMelting |
There was a problem hiding this comment.
The organization you have for these tests will work fine. It's pretty different from what we do on the ocean side and it seems hard to read (at least for me) with the combination of -, /, _ and camelCase.
A couple of suggestions for your consideration. Maybe make more subdirectories. That way, you don't need - anymore. Maybe assume mesh is implied. Maybe shorten decomposition_test to decomp or similar and restart_test to restart since the path is so long. Maybe use sia rather than none for velo (if that's accurate) so you can eliminate velo itself from the path. Maybe nocalving or no_calving instead of none so you can have the others without calving. Finally, it might make sense to have decomp, restart, etc. at the the inner-most level. This would allow you to have varying levels of subdirectories, where the default (e.g. no calving or no face melting) is implied. I'm not sure what to do about damage---the best I've come up with is damage_threshold (using underscores for consistency with my other suggestions) but it could still be damage-threshold if you prefer.
:
landice/humboldt/3km/sia/decomp
landice/humboldt/3km/sia/floating/decomp
landice/humboldt/3km/sia/eigencalving/decomp
landice/humboldt/3km/sia/specified_calving_velocity/decomp
landice/humboldt/3km/sia/damagecalving/decomp
landice/humboldt/3km/sia/ismip6_retreat/decomp
landice/humboldt/3km/sia/von_mises_stress/decomp
landice/humboldt/3km/sia/von_mises_stress/damage_threshold/face_melting/decomp
Again, it's okay if you want to keep what you have. These are not tests I expect to interact with very often so it's mostly about what works well for you all.
There was a problem hiding this comment.
@xylar , Trevor and I discussed this at length and went back and forth a number of times. In the end we decided to stick with what we have, despite the confusing and verbose aspects to it. The flat structure was intentional for this large set of tests to make it easier to keep track of them all without needing to cd across a number of levels. Some of the details of the symbol usage could be improved, but a number of the idiosyncrasies you pointed out have to do with how options are defined in MALI's Registry, so we decided to leave as is for consistency with that. All of that said, if after using these sets of tests more we find the organizational structure feels awkward, we can revise in a future PR.
There was a problem hiding this comment.
Thanks, that makes sense. I appreciate you thinking through the alternatives. I certainly appreciate the advantage of the flat directory structure.
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-eigencalving | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-specified_calving_velocity | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-von_mises_stress | ||
| landice/humboldt/mesh-3km_decomposition_test/velo-none_calving-damagecalving |
There was a problem hiding this comment.
Maybe this makes sense from MALI naming conventions but should it be damage_calving for consistency?
There was a problem hiding this comment.
For various reasons that option happens to be spelled damagecalving in MALI.
|
|
||
| # Set up tests without calving using the 1km mesh | ||
| mesh_type = '1km' | ||
| for velo_solver in ['FO', ]: |
There was a problem hiding this comment.
Presumably, the 1km mesh is purposefully being run only with the FO solver.
BTW, the syntax you have isn't wrong but it's unusual. This would be more typical:
| for velo_solver in ['FO', ]: | |
| for velo_solver in ['FO']: |
| subdir = 'mesh-{}_decomposition_test/velo-{}_calving-{}'.format( | ||
| mesh_type, velo_solver.lower(), calving_law.lower()) | ||
| # append damage and facemelt if provided | ||
| if damage is not None: | ||
| subdir += '_damage-{}'.format(damage) | ||
| if face_melt is True: | ||
| subdir += '_faceMelting' |
There was a problem hiding this comment.
Here and elsewhere, please consider using f-strings rather than the .format() method for (in my opinion) better readability:
| subdir = 'mesh-{}_decomposition_test/velo-{}_calving-{}'.format( | |
| mesh_type, velo_solver.lower(), calving_law.lower()) | |
| # append damage and facemelt if provided | |
| if damage is not None: | |
| subdir += '_damage-{}'.format(damage) | |
| if face_melt is True: | |
| subdir += '_faceMelting' | |
| subdir = f'mesh-{mesh_type}_decomposition_test/' \ | |
| f'velo-{velo_solver.lower()}_calving-{calving_law.lower()}' | |
| # append damage and facemelt if provided | |
| if damage is not None: | |
| subdir = f'{subdir}_damage-{damage}' | |
| if face_melt is True: | |
| subdir = f'{subdir}_faceMelting' |
There was a problem hiding this comment.
I removed all the hanging commas, but left the .format strings. I'll switch to using f-strings in the future.
| else: | ||
| self.proc_list = [1, 32] | ||
| for procs in self.proc_list: | ||
| name = '{}proc_run'.format(procs) |
There was a problem hiding this comment.
| name = '{}proc_run'.format(procs) | |
| name = f'{procs}proc_run' |
I guess you get the idea...
|
|
||
| elif self.velo_solver == 'FO': | ||
| # validate thickness | ||
| variable = ['thickness', ] |
There was a problem hiding this comment.
| variable = ['thickness', ] | |
| variable = ['thickness'] |
I'm guessing this syntax comes from doing the same for tuples. That's necessary for tuples to distinguish between them and "normal" parentheses for a tuple with one item but it isn't needed for lists (or sets, I believe).
|
|
||
| <stream name="output" | ||
| output_interval="0000-00-01_00:00:00" | ||
| reference_time="0000-01-01_00:00:00" |
There was a problem hiding this comment.
Yeah, that's confusing that should be fixed.
| calving_law: {'none', 'floating', 'eigencalving', | ||
| 'specified_calving_velocity', 'von_Mises_stress', | ||
| 'damagecalving', 'ismip6_retreat'}, optional | ||
| The calving law setting to use for this test case. If not | ||
| specified, set to 'none'. |
There was a problem hiding this comment.
I wish this worked. The docstring doesn't parse correctly:
Instead, I'd suggest:
| calving_law: {'none', 'floating', 'eigencalving', | |
| 'specified_calving_velocity', 'von_Mises_stress', | |
| 'damagecalving', 'ismip6_retreat'}, optional | |
| The calving law setting to use for this test case. If not | |
| specified, set to 'none'. | |
| calving_law: str, optional | |
| The calving law setting to use for this test case. If not | |
| specified, set to 'none'. Valid values are: 'none', | |
| 'floating', 'eigencalving', 'specified_calving_velocity', | |
| 'von_Mises_stress', 'damagecalving', 'ismip6_retreat'} |
| humboldt | ||
| ~~~~~~~~ |
This commit adds a new composition test for the humboldt test group and applies it to a range of calving laws. The structure is based on the dome decomposition test with the added argument for calving law. This commit adds this new decomposition test to a combination of different calving laws. The code runs but there are a number of details that need cleaning up in subsequent commits.
Using the mesh generation step led to some issues: * There was no muFriction or uReconstructX/Y field * The mesh generation adds a few minutes of run time to each test In the future we may try to fold the mesh generation back in, but for now it is easier to separate these operations.
Not all pass, but being able to run them all en masse is very useful.
* Add the forcing file required by some of the calving laws * improve durations of the various tests to ensure things happen * add more restart tests * make some validation fields conditional on options applied * set restart run to use same # cores for both steps
Running the 1km Humboldt mesh broke since I created this PR due to the updated basal b.c. in Albany. This commit updates the yaml file to use the correct basal b.c. settings for the current version.
This commit adds a 3km version of the Humboldt mesh for the calving tests. It introduces a mesh_type that can either be 1km or 3km. It makes uses of a streams template to set the appropriate input files. It also updates the tests in the calving integration suite to use the 3km version, but leaves the 1km tests in the list of available tests.
The default value results in calving that is too large for this domain, leading to errors. This change reduces the calving parameter, allowing the eigencalving tests to pass while still producing meaningful calving rates.
This changes the comparison tolerances for the decomposition FO tests to that required to account for the non-BFB decomp behavior of the FO solver. i.e. the tolerances are set to those required for a run with no calving to pass. I also updated the eigencalving parameter to be smaller to keep the uncalved error from killing the run. The previous value was small enough for running without velocity solver, but was still large once the FO solver was turned on. With these changes, all tests in the FO calving suite pass, with the exception of landice_humboldt_mesh-3km_decomposition_test_velo-fo_calving-ismip6_retreat which is a known problem to be addressed in MALI at a later date.
Now that we have a bunch of tests that run MALI, default was too generic.
These test additionally enable facemelting and damage calving, in addition to having calving enabled. These are meant to represent the most physics-rich configurations MALI currently supports and provide COMPASS tests that exercise all the physics we support. There currently is a decomposition test and a restart test of this configuration, both with von Mises for calving. There are versions for both using either the FO velocity solver or no velocity solver. The ability to add these two additional options has been done in a general way to allow other combinations easily later if desired. These have been added to the respective calving integration suites.
This commit makes tests run faster in two ways: 1. Restart tests only integrate one year forward after the restart. There is no need to run a long time after the restart to ensure a BFB-restart. There is an implicit assumption here that the decomposition tests will also be run and serve as a longer integrating smoke test in addition to a decomp test. 2. For the 'full physics' configurations, increase the time step from 4 months to 6 months on the 3km mesh so they run a little faster. This is to allow them to run in the full_integration suite without adding too much additional time.
The decomp version is known to fail right now and requires debugging MALI to fix. We will wait to add it to the integration suite until the problem has been fixed.
The ISMIP6 forcings are defined on July 1 of each year, and at one point, there was an issue with reading forcings requiring that the start time match the forcing time on init. That problem was fixed in MALI-Dev/E3SM#10 so we no longer need to start in the middle of the year to use the ISMIP6 forcing. To avoid confusion, I'm changing all tests to starrt on 2007-01-01. Also change all stream reference times to 2007-01-01. This is not necessary, because the default reference time of 0000-01-01 will yield the same results. But I'm changing it for explicitness. A minor conisderation is choosing a reference_time closer to the model clock time should provide a very small improvement to the cost of timekeeping operations related to i/o.
matthewhoffman
force-pushed
the
landice/add_calving_law_tests
branch
from
25ba2b0 to
66544ff
Compare
This merge introduces decomposition and restart tests to the Humboldt test group. It also adds the ability to test them using different velocity solvers and calving laws. It then creates test suites for the various calving law tests. There are enough of these that are expensive enough that we don't want to add them all to our standard regression suites. The new suites allow bulk testing of calving options that can be run periodically when changes to the calving code occur. Note that not all of these tests pass due to known (or now exposed) issues in MALI. Indeed, a primary purpose of this PR is to expose these sort of bugs. Failures in the calving integration suites will be resolved in MALI in the future.
Additionally, a set of "full physics" configurations are created that also include damage threshold calving and facemelting. These configurations are meant to represent the fullest set of physics we currently use in MALI for a realistic mesh. These full physics tests are added to he calving regression suites, as well as the standard integration suites.