-
Notifications
You must be signed in to change notification settings - Fork 446
Homme(xx)/SL: Finish C++/Kokkos for ETM; modify vertical discretization. #7807
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
Conversation
|
Testing is finished:
|
| <hypervis_subcycle_q hgrid=".*pg2">6</hypervis_subcycle_q> | ||
| <transport_alg hgrid=".*pg2">12</transport_alg> | ||
| <semi_lagrange_trajectory_nsubstep>0</semi_lagrange_trajectory_nsubstep> | ||
| <semi_lagrange_trajectory_nvelocity>-1</semi_lagrange_trajectory_nvelocity> |
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.
These permit automatic defaults to be computed.
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.
Would you mind adding the doc="..." string to these parameters, with a brief explanation? To help users know if/when they want to modify these params. Also, if some params have a limited set of valid values, you could also set the valid_values="a,b,c" metadata as well.
| ATMCHANGE=$CIMEROOT/../components/eamxx/scripts/atmchange | ||
|
|
||
| $ATMCHANGE semi_lagrange_trajectory_nsubstep=2 -b | ||
| $ATMCHANGE semi_lagrange_trajectory_nvelocity=3 -b |
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.
Update a test now that nvelocity > 2 is implemented for EAMxx.
| #AOT flags | ||
| SET(SYCL_COMPILE_FLAGS "-std=c++17 -fsycl -fsycl-device-code-split=per_kernel -fno-sycl-id-queries-fit-in-int -fsycl-unnamed-lambda -Xclang -fsycl-allow-virtual-functions") | ||
| #SET(SYCL_COMPILE_FLAGS "-std=c++17 -fsycl -fsycl-device-code-split=per_kernel -fno-sycl-id-queries-fit-in-int -fsycl-unnamed-lambda -Xclang -fsycl-allow-virtual-functions") | ||
| SET(SYCL_COMPILE_FLAGS "-std=c++17 -fsycl -fsycl-device-code-split=per_kernel -fno-sycl-id-queries-fit-in-int -fsycl-unnamed-lambda") |
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.
This is for standalone-Homme builds on Aurora. It looks like -Xclang -fsycl-allow-virtual-functions is now neither supported nor needed. I'm leaving the previous line commented out in case another compiler version change reinstates this flag and its necessity.
| SET (ADD_Fortran_FLAGS "-traceback" CACHE STRING "") | ||
| SET (ADD_C_FLAGS "-traceback" CACHE STRING "") | ||
| SET (ADD_CXX_FLAGS "-traceback" CACHE STRING "") | ||
| SET (BUILD_HOMME_THETA_KOKKOS TRUE CACHE BOOL "") |
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.
This enables C++/Kokkos testing in the optimized test of homme_integration. Previously, only the BFB test would build the C++/Kokkos components. The optimized test will now run some unit tests and tracer transport tests that have analytical solutions and checks on those solutions.
| @@ -0,0 +1,39 @@ | |||
| # This utility checks output of the form | |||
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.
Support homme_integration transport-test error checking in the case of known solutions.
|
|
||
| IF (HOMME_ENABLE_COMPOSE) | ||
| LIST(APPEND HOMME_TESTS | ||
| thetanh-moist-bubble-sl.cmake |
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 had to rearrange some lists to support running C++/Kokkos tests in the optimized homme_integration test in addition to the BFB test.
| createTests(HOMME_TESTS) | ||
|
|
||
| IF (${BUILD_HOMME_PREQX_KOKKOS}) | ||
| IF (HOMMEXX_BFB_TESTING AND BUILD_HOMME_PREQX_KOKKOS) |
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.
More changes to support C++/Kokkos in the homme_integraiton optimized test.
| @@ -0,0 +1,3 @@ | |||
| THETAL_KOKKOS_SETUP() | |||
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.
This is for the new planar-transport test.
| #ifndef CAM | ||
| #include "config.h" | ||
|
|
||
| module planar_transport_tests |
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.
New planar-transport test, paralleling the sphere tests. Just like the recently added sphere test, surface pressure varies in space and time.
| test_case(1:13)== "jw_baroclinic" .or. & | ||
| test_case(1:5) == "dcmip" .or. & | ||
| test_case(1:5) == "mtest" .or. & | ||
| test_case == "planar_hydro_gravity_wave" .or. & |
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.
Clean up needless checks. This follows the pattern of the other test sets: check just the prefix.
|
@bartgol I opened this PR for a branch on my fork. Is there something special I need to do to get CI to work? Thanks. |
Gitlab-ex is having some SSL certificate issue, which is affecting all AT2 customers as well. Hopefully it will get fixed soon. Once it's back online, I will make sure testing runs on this PR. |
bartgol
left a comment
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.
Looks good. I only have a minor request, for the sake of users.
| <hypervis_subcycle_q hgrid=".*pg2">6</hypervis_subcycle_q> | ||
| <transport_alg hgrid=".*pg2">12</transport_alg> | ||
| <semi_lagrange_trajectory_nsubstep>0</semi_lagrange_trajectory_nsubstep> | ||
| <semi_lagrange_trajectory_nvelocity>-1</semi_lagrange_trajectory_nvelocity> |
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.
Would you mind adding the doc="..." string to these parameters, with a brief explanation? To help users know if/when they want to modify these params. Also, if some params have a limited set of valid values, you could also set the valid_values="a,b,c" metadata as well.
|
The CI testing looks good. The CI EAMxx tests are failing with NLFAIL due to expected This helpful message from the CI shows that this is the only reason for the failure; I checked that this indeed is true in a downloaded artifact for one of the tests. |
|
I'll merge this when |
|
There are conflicts with |
Ah, sorry, it may have been my PR that removed old unused targets. |
Sure, that's fine. I was just noting what I'm waiting for. |
The atmosphere semi-Lagrangian tracer transport method's enhanced trajectory method (ETM) is a stealth feature intended to make tracer transport time steps even longer and, in any case, more flexible. This PR completes the C++/Kokkos version of the ETM, in particular support for semi_lagrange_trajectory_nvelocity > 2. It also modifies the vertical discretization relative to the original ETM implementation, in light of issues at and around the tropopause when using the ETM in test v3 simulations. This new approach resolves the tropopause issue. Finally, this PR strengthens the homme_integration test suite as follows: * add a new planar tracer transport test with analytical solution at the end time, with space- and time-varying surface pressure; * add instances of this test to the homme_integration test suite; * add a cmake tool to check error output for the recently added sphere and new plane tracer transport tests when running this suite; * activate the Kokkos builds in the non-BFB part of homme_integration, turning on the parts of the unit tests that are meaningful in the non-BFB build and the recently added and new tracer transport tests.
61340a2 to
4a4c944
Compare
Homme(xx)/SL: Finish C++/Kokkos for ETM; modify vertical discretization. The atmosphere semi-Lagrangian tracer transport method's enhanced trajectory method (ETM) is a stealth feature intended to make tracer transport time steps even longer and, in any case, more flexible. This PR completes the C++/Kokkos version of the ETM, in particular support for semi_lagrange_trajectory_nvelocity > 2. It also modifies the vertical discretization relative to the original ETM implementation, in light of issues at and around the tropopause when using the ETM in test v3 simulations. This new approach resolves the tropopause issue. Finally, this PR strengthens the homme_integration test suite as follows: * add a new planar tracer transport test with analytical solution at the end time, with space- and time-varying surface pressure; * add instances of this test to the homme_integration test suite; * add a cmake tool to check error output for the recently added sphere and new plane tracer transport tests when running this suite; * activate the Kokkos builds in the non-BFB part of homme_integration, turning on the parts of the unit tests that are meaningful in the non-BFB build and the recently added and new tracer transport tests. [non-BFB] new tests and existing tests of the ETM [BFB] all other tests
|
Holding on |
|
@bartgol what do I need to do to clear the NLFAILs in the EAMxx test baselines used for the autotester? Or is that automatic when the PR goes into master? Thanks. |
You need to go on the actions tab at the top of the page, and look for the eamxx-v1 workflow on the left. Then, on the top right, click on the drop down menu "Run workflow". It will open a small menu, where you can select the branch from which the workflow should run, the jobs to run, and whether to gen baselines. You should leave 'master' for the branch, ofc, and check the "generate baselines" box. The jobs to run is currently irrelevant, as there is only machine were we run v1 tests with gh actions (the "all" option does the same thing as "cpu-gcc"). |
The atmosphere semi-Lagrangian tracer transport method's enhanced trajectory method (ETM) is a stealth feature intended to make tracer transport time steps even longer and, in any case, more flexible.
This PR completes the C++/Kokkos version of the ETM, in particular support for semi_lagrange_trajectory_nvelocity > 2.
It also modifies the vertical discretization relative to the original ETM implementation, in light of issues at and around the tropopause when using the ETM in test v3 simulations. This new approach resolves the tropopause issue.
Finally, this PR strengthens the homme_integration test suite as follows:
[non-BFB] new tests and existing tests of the ETM
[BFB] all other tests