Homme(xx)/pgN: Add pgN feature to doubly periodic planar mode. #6057

ambrad · 2023-11-06T19:35:59Z

Add pgN feature to planar doubly periodic mode.
Add some standalone-hommexx infrastructure to support using the same unit-test exe in multiple tests.
Add an init-planar capability to Hommexx unit tests.
Add a unit test: gllfvremap_planar_ut, planar version of old gllfvremap_ut unit test.
Add a BFB test for F90/C++ dycore consistency in planar mode with pg2 physics grid.

[BFB] except for new tests in homme_integration

ambrad · 2023-11-06T19:37:07Z

Testing:

Chrysalis homme_integration: pass except baseline check because of new tests
Chrysalis e3sm_developer: pass
BFB tests on Ascent/Summit: pass
Test merge into SCREAM repo on Chrysalis: e3sm_scream_v1 passes against baselines for a base repo from prior to the recent upstream merge
Test merge into SCREAM repo on Ascent: pass (ERS_Ln362_P6x1.ne30pg2_ne30pg2.F2010-SCREAMv1.ascent_gnugpu)

bartgol · 2023-11-06T22:07:49Z

components/homme/src/share/cxx/GllFvRemapImpl.cpp

@@ -514,6 +514,7 @@ ::run_dyn_to_fv_phys (const int timeidx, const Phys1T& ps, const Phys1T& phis, c
      kv.team_barrier(); // w2, w3, w4 in use
      // T_f
      loop_ik(ttrf, tvr, [&] (int i, int k) { T(ie,i,k) = th_f(i,k)*exner_f(i,k); });
+      kv.team_barrier(); // w2, w3 in use


I don't understand why we need this barrier (and the one below)

Neither is clearly needed. However, because there is still a potential source of very rare nondeterminism lurking in our code, I've been inspecting old code as I work on new related code. All the data I've collected about this rare nondeterminism can't rule out its occurring in physics-dynamics grid remap, so I read through this file again while working on the planar physgrid case. I decided to put these two barriers in based on this fresh read-through.

The one below, in my analysis, is needed if uv_dim != 3, but I decided to put it in w/o a check on that condition for the reason I'll state in a minute. (In practice, uv_dim == 3.)

I put this one in the for the same reason, although here again the loop patterns in the two adjacent loops arguably don't require it, though in a much subtler way than the usual case of identical loop patterns.

The principle I decided to follow here was strict adherence to protecting work arrays, disregarding implicit protection from loop patterns, just to make reasoning about determinism here simpler. I believe with these two additional team_barriers, that principle is then followed throughout the whole file.

oksanaguba · 2023-11-07T02:38:39Z

components/homme/src/share/gllfvremap_util_mod.F90

@@ -536,6 +591,8 @@ function gfr_check_api(hybrid, nets, nete, hvcoord, elem) result(nerr)
    integer :: nphys, ftype_in, ftype_idx, boost_idx, nerr
    logical :: boost_pg1

+    is_sphere = trim(geometry) /= 'plane'


unrelated to this code, but why do we have both geometry and topology in control_mod?

if (topology == "cube") then write(iulog,*)"readnl: ne,np = ",ne,np else if (topology == "plane") then write(iulog,*)"readnl: ne_x,ne_y,np = ",ne_x,ne_y,np end if

Topology is for the grid, geometry for the metric terms. Chris did that.

oksanaguba · 2023-11-07T02:43:08Z

components/homme/test_execs/thetal_kokkos_ut/CMakeLists.txt

@@ -347,3 +347,4 @@ ELSE()
 ENDIF()
 cxx_unit_test (gllfvremap_ut "${GLLFVREMAP_UT_F90_SRCS}" "${GLLFVREMAP_UT_CXX_SRCS}" "${GLLFVREMAP_UT_INCLUDE_DIRS}" "${CONFIG_DEFINES}" ${NUM_CPUS})
 TARGET_LINK_LIBRARIES(gllfvremap_ut thetal_kokkos_ut_lib)
+cxx_unit_test_add_test(gllfvremap_planar_ut gllfvremap_ut ${NUM_CPUS} "hommexx -planar")


I thought `cxx_unit_test_add_test' macro is just to go around mpiexec in cxx_unit_test, but here it is called standalone. why?

This is to call the same unit test exe twice with different inputs. Before, you could only call it once or else have to create a new exe, meaning another build.

oksanaguba · 2023-11-07T02:44:39Z

components/homme/src/share/gllfvremap_mod.F90

@@ -2175,6 +2208,29 @@ subroutine limiter_clip_and_sum(spheremp, qmin, qmax, dp, q)
    call limiter1_clip_and_sum(np*np, spheremp, qmin, qmax, dp, q)
  end subroutine limiter_clip_and_sum

+  subroutine ref2plane(corners, a, b, plane)


i assume this is needed for SL?

This is for the metric terms in dynamics-physics grid remap. This PR is to support pg2 in planar mode.

oksanaguba · 2023-11-07T02:45:39Z

components/homme/src/share/gllfvremap_util_mod.F90

@@ -168,14 +208,29 @@ subroutine set_gll_state(hvcoord, elem, nt1, nt2)
    end do
  end subroutine set_gll_state

+  function make_tendency(p) result(t)


Is this a forcing function tied to the special Cinf IC? If not, what is this for?

This is for gllfvremap internal functional property unit tests. I suggest ignoring this.

oksanaguba · 2023-11-07T02:48:26Z

components/homme/test/reg_test/run_tests/test-list.cmake

-    thetanh-moist-bubble-sl-kokkos.cmake)
+    thetanh-moist-bubble-sl-kokkos.cmake
+    thetanh-moist-bubble-sl-pg2.cmake
+    thetanh-moist-bubble-sl-pg2-kokkos.cmake)


it seems pg2 tests in this list are planar, can we use planar in their names?

I can, but please note I'm following the naming scheme that I believe you established.

oksanaguba · 2023-11-07T02:50:50Z

regarding "Add some standalone-hommexx infrastructure to support using the same uni-test exe in multiple tests." -- can you elaborate?

"Add a unit test: gllfvremap_planar_ut." -- which part does it test?

"Add a BFB test for F90/C++ dycore consistency." -- i assume this is for a planar pg2 test, can i modify the PR description?

thanks!

ambrad · 2023-11-07T02:59:11Z

regarding "Add some standalone-hommexx infrastructure to support using the same uni-test exe in multiple tests." -- can you elaborate?

This is to make it possible to call a unit test exe multiple times with different inputs, permitting e.g. cube-sphere and then planar runs without building another exe.

"Add a unit test: gllfvremap_planar_ut." -- which part does it test?

It's the long-standing gllfvremap_ut test extended to planar geometry and doubly periodic topology.

"Add a BFB test for F90/C++ dycore consistency." -- i assume this is for a planar pg2 test, can i modify the PR description?

Done.

oksanaguba

thanks

regarding renaming tests -- it is a minor thing, up to you (i am sure i will forget which ones are planar next time, but i will read nl files).

This is an old bug in the original forcing function. The physgrid variant, dcmip2016_test1_forcing, is fine.

Homme(xx)/pgN: Add pgN feature to doubly periodic planar mode. * Add pgN feature to planar doubly periodic mode. * Add some standalone-hommexx infrastructure to support using the same unit-test exe in multiple tests. * Add an init-planar capability to Hommexx unit tests. * Add a unit test: gllfvremap_planar_ut, planar version of old gllfvremap_ut unit test. * Add a BFB test for F90/C++ dycore consistency in planar mode with pg2 physics grid. e3sm_developer passes on Chrysalis. Additional tests for GPU/EAMxx pass on various machines. [BFB] except for new tests in homme_integration

ambrad added 4 commits November 6, 2023 12:39

Homme: Modify dcmip2016_test1_pg_forcing to handle planar case.

206a093

Homme: Add planar capability to gllfvremap_mod.

4b54afa

Homme(xx): Set up C++-vs-F90 standalone planar pg2 test.

804b3b9

Homme: Add gllfvremap_planar_ut unit test.

e1a1356

ambrad requested review from oksanaguba and bartgol November 6, 2023 19:36

ambrad self-assigned this Nov 6, 2023

ambrad added HOMME EAMxx PRs focused on capabilities for EAMxx labels Nov 6, 2023

bartgol reviewed Nov 6, 2023

View reviewed changes

bartgol approved these changes Nov 6, 2023

View reviewed changes

oksanaguba reviewed Nov 7, 2023

View reviewed changes

oksanaguba approved these changes Nov 7, 2023

View reviewed changes

Homme: Fix latitude in dcmip2016_test1_forcing.

50d641d

This is an old bug in the original forcing function. The physgrid variant, dcmip2016_test1_forcing, is fine.

ambrad force-pushed the ambrad/homme/dp-pg branch from 8792299 to 50d641d Compare November 7, 2023 20:11

Homme/SL: Address a warning.

8324c66

ambrad merged commit f4fef0b into E3SM-Project:master Nov 14, 2023
2 checks passed

ambrad mentioned this pull request Aug 6, 2024

Update DPxx tests to run with PG2 grid E3SM-Project/scream#2940

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Homme(xx)/pgN: Add pgN feature to doubly periodic planar mode. #6057

Homme(xx)/pgN: Add pgN feature to doubly periodic planar mode. #6057

ambrad commented Nov 6, 2023 •

edited

Loading

ambrad commented Nov 6, 2023 •

edited

Loading

bartgol Nov 6, 2023

ambrad Nov 6, 2023 •

edited

Loading

oksanaguba Nov 7, 2023

ambrad Nov 7, 2023

oksanaguba Nov 7, 2023

ambrad Nov 7, 2023

oksanaguba Nov 7, 2023

ambrad Nov 7, 2023

oksanaguba Nov 7, 2023

ambrad Nov 7, 2023

oksanaguba Nov 7, 2023

ambrad Nov 7, 2023

oksanaguba commented Nov 7, 2023 •

edited

Loading

ambrad commented Nov 7, 2023

oksanaguba left a comment

Homme(xx)/pgN: Add pgN feature to doubly periodic planar mode. #6057

Homme(xx)/pgN: Add pgN feature to doubly periodic planar mode. #6057

Conversation

ambrad commented Nov 6, 2023 • edited Loading

ambrad commented Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

ambrad Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oksanaguba commented Nov 7, 2023 • edited Loading

ambrad commented Nov 7, 2023

oksanaguba left a comment

Choose a reason for hiding this comment

ambrad commented Nov 6, 2023 •

edited

Loading

ambrad commented Nov 6, 2023 •

edited

Loading

ambrad Nov 6, 2023 •

edited

Loading

oksanaguba commented Nov 7, 2023 •

edited

Loading