8000 Improve hydro_radial test cases by matthewhoffman · Pull Request #811 · MPAS-Dev/compass · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Improve hydro_radial test cases #811

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 1, 2024
Merged

Improve hydro_radial test cases #811

merged 3 commits into from
May 1, 2024

Conversation

matthewhoffman
Copy link
Member
@matthewhoffman matthewhoffman commented Apr 4, 2024 •
edited
Loading

This merge adds improvements to the hydro_radial test cases to make them more useful

  • adjust decomposition_test to exactly perform the one-month drift
    test that steady_state_drift_test runs (results of this are in the
    MALI GMD paper and the Bueler and van Pelt paper)
  • adjust restart_test to perform the drift test for two months instead
    of initializing to no water and running for two years
  • set Visualize step to always be run for all four tests
  • Let spinup, drift test and restart test use up to 128 procs (but keep
    decomp at 1 and 3)

Checklist

  • User's Guide has been updated
  • Developer's Guide has been updated
  • Documentation has been built locally and changes look as expected
  • Document (in a comment titled Testing in this PR) any testing that was used to verify the changes

@matthewhoffman matthewhoffman added enhancement New feature or request land ice in progress This PR is not ready for review or merging labels Apr 4, 2024
@matthewhoffman
Update hydro_radial test to be more useful
b0939c8 
* adjust decomposition_test to exactly perform the one-month drift
  test that steady_state_drift_test runs (results of this are in the
  MALI GMD paper and the Bueler and van Pelt paper)
* adjust restart_test to perform the drift test for two months instead
  of initializing to no water and running for two years
* set Visualize step to always be run for all four tests
* Let spinup, drift test and restart test use up to 128 procs (but keep
  decomp at 1 and 3)
@matthewhoffman
Update docs
aa437b4
@matthewhoffman matthewhoffman force-pushed the landice/hydro_radial_improvements branch from aa05d44 to aa437b4 Compare April 4, 2024 21:40
@matthewhoffman
Copy link
Member Author
matthewhoffman commented Apr 4, 2024 •
edited
Loading

Testing

I set up the four tests using:
compass setup -n 85 86 88 87 -w $scratch5/compass-hydro-radial-tests

Results:

landice/hydro_radial/decomposition_test
  * step: setup_mesh
  * step: 1proc_run
  * step: visualize_1proc_run
  * step: 3proc_run
  * step: visualize_3proc_run
  test execution:      SUCCESS
  test validation:     PASS
  test runtime:        00:11
landice/hydro_radial/restart_test
  * step: setup_mesh
  * step: full_run
  * step: visualize_full_run
  * step: restart_run
  * step: visualize_restart_run
  test execution:      SUCCESS
  test validation:     PASS
  test runtime:        00:54
landice/hydro_radial/steady_state_drift_test
  * step: setup_mesh
  * step: run_model
  * step: visualize
  test execution:      SUCCESS
  test runtime:        00:10
landice/hydro_radial/spinup_test
  * step: setup_mesh
  * step: run_model
  * step: visualize
  test execution:      SUCCESS
  test runtime:        09:18
Test Runtimes:
00:11 PASS landice_hydro_radial_decomposition_test
00:54 PASS landice_hydro_radial_restart_test
00:10 PASS landice_hydro_radial_steady_state_drift_test
09:18 PASS landice_hydro_radial_spinup_test
Total runtime 10:35
PASS: All passed successfully!

All looks good, but I'm surprised landice/hydro_radial/restart_test takes nearly a minute. Might be a fluke so I will check what it is when I run the full_integration suite.

@matthewhoffman
Copy link
Member Author

Testing, Part 2

Ran full_integration suite on Chicoma against compass/main (daff021). Both use E3SM repo hash 7067f38968.
Results:

Test Runtimes:
00:12 PASS landice_dome_2000m_sia_restart_test
00:04 PASS landice_dome_2000m_sia_decomposition_test
00:04 PASS landice_dome_variable_resolution_sia_restart_test
00:02 PASS landice_dome_variable_resolution_sia_decomposition_test
00:21 PASS landice_enthalpy_benchmark_A
00:24 PASS landice_eismint2_decomposition_test
00:23 PASS landice_eismint2_enthalpy_decomposition_test
00:24 PASS landice_eismint2_restart_test
00:24 PASS landice_eismint2_enthalpy_restart_test
00:12 PASS landice_greenland_sia_restart_test
00:11 PASS landice_greenland_sia_decomposition_test
00:19 FAIL landice_hydro_radial_restart_test
00:11 FAIL landice_hydro_radial_decomposition_test
00:34 PASS landice_humboldt_mesh-3km_decomposition_test_velo-none_calving-none_subglacialhydro
00:36 PASS landice_humboldt_mesh-3km_restart_test_velo-none_calving-none_subglacialhydro
00:19 PASS landice_dome_2000m_fo_decomposition_test
00:17 PASS landice_dome_2000m_fo_restart_test
00:10 PASS landice_dome_variable_resolution_fo_decomposition_test
00:10 PASS landice_dome_variable_resolution_fo_restart_test
00:15 PASS landice_circular_shelf_decomposition_test
00:53 PASS landice_greenland_fo_decomposition_test
00:44 PASS landice_greenland_fo_restart_test
00:25 PASS landice_thwaites_fo_decomposition_test
00:32 PASS landice_thwaites_fo_restart_test
00:13 PASS landice_thwaites_fo-depthInt_decomposition_test
00:19 PASS landice_thwaites_fo-depthInt_restart_test
00:37 PASS landice_humboldt_mesh-3km_restart_test_velo-fo_calving-von_mises_stress_damage-threshold_faceMelting
00:20 PASS landice_humboldt_mesh-3km_restart_test_velo-fo-depthInt_calving-von_mises_stress_damage-threshold_faceMelting
Total runtime 09:38
FAIL: 2 tests failed, see above.

The two hydro_radial tests pass validation but fail baseline comparison as expected and can be blessed:

landice/hydro_radial/restart_test
  * step: setup_mesh
  * step: full_run
  * step: visualize_full_run
  * step: restart_run
  * step: visualize_restart_run
  test execution:      SUCCESS
  test validation:     PASS
  baseline comparison: FAIL
  see: case_outputs/landice_hydro_radial_restart_test.log
  test runtime:        00:19
landice/hydro_radial/decomposition_test
  * step: setup_mesh
  * step: 1proc_run
  * step: visualize_1proc_run
  * step: 3proc_run
  * step: visualize_3proc_run
  test execution:      SUCCESS
  test validation:     PASS
  baseline comparison: FAIL
  see: case_outputs/landice_hydro_radial_decomposition_test.log
  test runtime:        00:11

Note the landice/hydro_radial/restart_test run time is 19 seconds here, so the 58 seconds in the previous test was probably a fluke and can be ignored.

Note that the hydro_radial runtimes are a bit longer than the baseline. I ran the integration suite a second time and got essentially the same times for these two tests. Presumably the additional runtime comes mostly from calling the visualization step. I think the extra cost is acceptable, but we could disable the viz step again (but one could run it manually after the integration suite was run if needed.) @trhille , if you prefer that, let me know.

Baseline run times:

00:07 PASS landice_hydro_radial_restart_test
00:05 PASS landice_hydro_radial_decomposition_test

@matthewhoffman matthewhoffman self-assigned this Apr 5, 2024
@matthewhoffman matthewhoffman removed the in progress This PR is not ready for review or merging label Apr 8, 2024
@alexolinhager
Copy link

@matthewhoffman, this PR looks good to me, just found one minor typo. No changes to the script itself. I ran the full integration on Perlmutter and got the same results as you, apart from slightly longer run times for the test case. That seems like it was most likely a fluke because the baseline was no different.

Screenshot 2024-04-24 at 5 10 09 PM
Screenshot 2024-04-24 at 5 09 57 PM
Baseline:
Screenshot 2024-04-24 at 5 10 34 PM

alexolinhager
alexolinhager reviewed Apr 25, 2024
View reviewed changes
Copy link
@alexolinhager alexolinhager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewhoffman, this PR looks good to me, just found one minor typo. No changes to the script itself. I ran the full integration on Perlmutter and got the same results as you, apart from slightly longer run times for the test case. That seems like it was most likely a fluke because the baseline was no different.

Screenshot 2024-04-24 at 5 10 09 PM
Screenshot 2024-04-24 at 5 09 57 PM
Baseline:
Screenshot 2024-04-24 at 5 10 34 PM

docs/users_guide/landice/test_groups/hydro_radial.rst Outdated
restart file saved by the first. Prognostic variables are compared between the
"full" and "restart" runs at year 2 to make sure they are bit-for-bit
identical.
"full" and "restart" runs atafter 2 to make sure they are bit-for-bit
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"atafter" --> "after year"

matthewhoffman
matthewhoffman commented Apr 28, 2024
View reviewed changes
@matthewhoffman matthewhoffman requested a review from trhille April 29, 2024 15:44
@matthewhoffman
Copy link
Member Author

@trhille , can you approve this PR? For some reason the automated checks are not working correctly - it says "Merging is blocked. Merging can be performed automatically with 1 approving review." but Alex has approved it. I checked the repo rules and only 1 review approval is required and there are no restrictions on who that is from. But that doesn't seem to be working so maybe another review approval will unstick it.

@trhille
Copy link
Collaborator
trhille commented Apr 29, 2024

I'm guess @alexolinhager doesn't have write access?

@alexolinhager
Copy link

Good question, how would I check that? I'm assuming that's the case because it works now that you approved changes

@matthewhoffman matthewhoffman merged commit 71ee2a7 into MPAS-Dev:main May 1, 2024
4 checks passed
@matthewhoffman
Copy link
Member Author

I didn't think write access is a requirement based on the Github docs. I'll try to dig deeper. In any case, thanks, @trhille

@matthewhoffman matthewhoffman deleted the landice/hydro_radial_improvements branch May 1, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trhille trhille trhille approved these changes

@alexolinhager alexolinhager alexolinhager approved these changes

Assignees

@matthewhoffman matthewhoffman

Labels
enhancement New feature or request land ice
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthewhoffman @alexolinhager @trhille
0