Repository navigation
Add MOAB coupler support for Omega - #523
Conversation
andrewdnolan
left a comment
andrewdnolan
left a comment
There was a problem hiding this comment.
@vijaysm Sorry this took me so long to get around to! Thanks for taking this on. Besides a few minor comments, this looks great!
While there aren't any conflicts; I think rebasing onto develop would be a good idea. We recently added support for G and B cases. Your thoughts of what sampling of test should use the moab vs. mct driver would be appreciated:
Lines 1150 to 1173 in c1aacdc
|
I'll do the rebase onto develop locally and start doing some testing! @vijaysm The instructions for installing the pre-commit environment are here: https://docs.e3sm.org/Omega/Omega/devGuide/Linting.html. With that you should be able to fix that failing CI test without much headache. |
|
@andrewdnolan I'll take care of these tonight or tomorrow. Thanks for the comments |
|
Sounds good! BTW, I did some testing using
|
|
Got it. I'll take a look at the code to see what is triggering it.
…On Tue, Sep 29, 2026 at 7:52 PM Andrew Nolan ***@***.***> wrote:
andrewdnolan left a comment (E3SM-Project/Omega#523)
Sounds good!
BTW, I did some testing using SMS.ne4_oQU240.WCYCL1850NS-OMEGA.chrysalis_intel:
First attempt had a build fail related to the outdated code in ocn_comp_mct.F90.
After deleting the code within the ifdefs I was able to get the case to build successfully. But, I go a runtime failure. CoPilot was suggesting it something related to the MPI communicator passed in iMOAB_RegisterApplication. Attaching the e3sm.log file here for reference. e3sm.log.1298248.260929-123040.log
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Detect MOAB via OMEGA_MOAB_ROOT when COMP_INTERFACE=moab, define HAVE_MOAB, and conditionally add/link MoabInterface.cpp. No effect when MOAB is absent.
registerApplication only registered an app. Now builds an owned-cells mesh (vertex compaction, polygon elements, GLOBAL_ID tags), domain tags, and x2o/o2x tag get/set. Also fixes header typos that blocked compilation.
Pass full CIME field-name lists to the C++ side, pick MOAB vs MCT layout at compile time, and call the mesh/tag-storage functions around ocean init and run. Drops the unused runtime layout selection it replaces.
moabInit's app id was staying local to the C++ side. The coupler-side migration/mapping code reads seq_comm_mct's mpoid to locate the ocean mesh, so leaving it at -1 meant it never found it, causing the crash and memory corruption seen at runtime. Fortran now copies the id into mpoid.
component_init_areacor_moab reads area:aream:mask as one combined tag; a missing tag fails the whole read. aream gets a -9999 placeholder, same convention MCT uses, since the coupler fills in the real value later.
iMOAB_CreateElements expects connectivity entries to be 1-based (it computes firstVertex + connectivity[j] - 1). Compact vertex indices were written 0-based, so every vertex reference pointed one slot too early.
6d6fe2b to
a03b0ac
Compare
|
@vijaysm Tried testing on chrysalis with:
Stack trace for the C-Case is: Looks like it's crashing during moab init. This is looking pretty similar to the stack trace in the log file linked above. When I previously had CoPilot look at the error and it suggested it something related to the MPI communicator passed in |
…s this is a mixed language usage scenario
andrewdnolan
left a comment
andrewdnolan
left a comment
There was a problem hiding this comment.
Testing
On chrysalis, I ran:
SMS.T62_oQU240.COMEGA-IAF.chrysalis_intel.20261007_110911_hylcq1
SMS.ne4pg2_oQU240.WCYCL1850NS-OMEGA.chrysalis_intel.20261007_134627_wy1es1
and both test pass! In order to use the ne4pg2_oQU240 grid alias I needed to cherry-pick e7e8960 from E3SM's master. Support for that grid alias will be brought into Omega's fork with #566.
@vijaysm Just a couple final comments. The only other thing I can think of is changing the tests in the test suite to run with MOAB:
Lines 1150 to 1173 in c1aacdc
We'll have to leave the
Vmct modifier on all the B-cases, until #566 is merged. Maybe we leave:
SMS_Vmct_Ld3.TL319_EC30to60E2r2.COMEGA-JRA1p5.omega-jra_1958
and
PEM_Vmct.T62_oQU240.GOMEGA-IAF
with the Vmct modifier, but then drop it from the rest of the C and G cases? Sorry, I'd go ahead a make those changes myself, but since this branch is from your fork I can't. If you want to pass that off, could you add me as a collaborator on your fork?
| ! Init import/export mct attribute vectors | ||
| ! Initialize MCT AVs with the ocean decomposition. The MOAB driver | ||
| ! uses their field lists during coupler-side initialization. | ||
| call mct_aVect_init(x2o, rList=seq_flds_x2o_fields, lsize=lsize) | ||
| call mct_aVect_init(o2x, rList=seq_flds_o2x_fields, lsize=lsize) |
There was a problem hiding this comment.
I get that these are still needed even when using the MOAB driver, but it seems like a waste to allocate these with a lsize=lsize. Could we just get way with lsize=1 when using MOAB? Or do they need to be full sized?
There was a problem hiding this comment.
I'm not exactly sure what to do about this; given it's what all other components do I think this is OK for now.
Will investigate using an lsize=1 when using MOAB at a later date.
There was a problem hiding this comment.
They shouldn't need to be full sized.
There was a problem hiding this comment.
I've tried with lsize=0 before and that fails. Yeah lsize=1 would be a good choice. Or we just create a better way to pass around field names without relying on MCT AVs?
There was a problem hiding this comment.
Agreed, the later seems like the best long term solution. But for now lsize=1 seems to work; at least of the C-Case. Will rerun the B-Case just to double check.
There was a problem hiding this comment.
The List datatype, which defines and operates on the colon delimited string that is part of an Av, can be used instead but doing that swap is something I was going to put off to the very end.
Maybe the latest Fortran standard has better ways to work with strings.
andrewdnolan
added
the
Coupled
Includes a comment cleanup
Testing,
|
andrewdnolan
left a comment
andrewdnolan
left a comment
There was a problem hiding this comment.
I'm approving based on inspection and testing on chrysalis.
|
@vijaysm @rljacob Should be expect BFB results between MOAB and MCT for all cases? I was able to get BFB results between the two for But Both tests were run with this branch and To set these up I just ran: # from cime/scripts
./create_test -t mct --driver mct SMS.ne4pg2_oQU240.WCYCL1850NS-OMEGA
./create_test -t moab --driver moab SMS.ne4pg2_oQU240.WCYCL1850NS-OMEGA
cd /lcrc/group/e3sm/ac.anolan/scratch/chrys/
cprnc \
SMS.ne4pg2_oQU240.WCYCL1850NS-OMEGA.chrysalis_intel.mct/run/SMS.ne4pg2_oQU240.WCYCL1850NS-OMEGA.chrysalis_intel.mct.cpl.hi.0001-01-06-00000.nc \
SMS.ne4pg2_oQU240.WCYCL1850NS-OMEGA.chrysalis_intel.moab/run/SMS.ne4pg2_oQU240.WCYCL1850NS-OMEGA.chrysalis_intel.moab.cpl.hi.0001-01-06-00000.ncand then the analogous command for There seems to be something about the |
Are there any custom commands that get run for this resolution? Can you also point me to the case that you set up? I'd like to see the diffs to understand which field is causing the error propagation. |
|
I ran similar tests but using MPASO instead of OMEGA. Those tests also showed diffs between the mct and moab runs, based on running cprnc on the coupler history files |
|
This is a C-case? The test case for that should be BFB between MCT and MOAB. That was T62_oQU120.CMPASO-NYF. Also T62_oQU120.CMPASO-IAF. Not sure we tested any other C-cases. |
|
@vijaysm there shouldn't be a need for any special commands on perlmutter. On chrysalis you'll need to apply this diff: diff --git a/cime_config/machines/config_machines.xml b/cime_config/machines/config_machines.xml
index 61a77a4766..a39f7715a7 100644
--- a/cime_config/machines/config_machines.xml
+++ b/cime_config/machines/config_machines.xml
@@ -2778,7 +2778,6 @@
<command name="load">subversion/1.14.0-e4smcy3</command>
<command name="load">perl/5.32.0-bsnc6lt</command>
<command name="load">cmake/3.24.2-whgdv7y</command>
- <command name="load">python/3.11.7-iexc6vq</command>
</modules>
<modules compiler="intel-classic">
<command name="load">intel/20.0.4-kodw73g</command>and then source: before running any CIME commands, to ensure pyyaml is available. After that nothing special; running Existing cases are: |
|
This is a C-case because it has a data sea-ice model. |
|
@rljacob Using the It seems that the Unfortunately I'm not seeing a grid alias for |
|
So I tested Jon's MCT/MOAB cases and they use the I would retry with |
|
Oh yeah VECT_MAP. There are some cases/machines where that doesn't matter but in some cases it does. |
|
Also, this is important and a good reason to move away from patch-based maps. Negative weights will notoriously induce spurious oscillatory modes when applied to fields with high gradients. Someone should do an analysis and replace the patch based maps with bilinear or something better. Anyway, none of this has to do with the Omega-MOAB interface. But just wanted to note this. |
|
Thanks for looking into that @vijaysm. After running |
|
With the
As a final tests I ran: coupler budgets are working as expected: |
jonbob
left a comment
jonbob
left a comment
There was a problem hiding this comment.
Approved based on testing and a visual inspection
|
Passes: |
|
Passes: |
|
Even though this shouldn't affect standalone runs, I ran Testing on auroraTested the merge of #523 (
aurora, intel, mpichPolaris
|
|
Yeah! Someone should make a PR to remove "Vmct" from the CIME-based tests. |
During coupled simulation standard output (stdout) is shared across all components and routed to e3sm.log. This PR adds an option to make stdout/stderr redirection to the omega log optional, which we need during coupled builds. Nothing about standalone logging in changed; redirection is true by default. This came up while testing #523, which is another C++ library. This has not come up previously, because all other coupled testing used all Fortran components. Closes #605
Update interfaces to MOAB coupler by defining the Omega mesh in MOAB, and pushing/pulling data out of tags by defining and updating the data as needed by the driver. Tested only with a C-case, but I think it should work with any other case as well since the interface is uniform from the component standpoint.
Checklist
Documentation:
Linting
Building
Testing
aurora, oneapi-ifx, mpich
chrysalis, oneapi-ifx, openmpi
frontier, craygnu, mpich
frontier, craygnu-mphipcc, mpich
pm-cpu, gnu, mpich
pm-gpu, gnugpu, mpich
Provide relevant details in a comment to the PR titled
Testingwith the following:have been run on and indicate that are all passing.
has passed, using the Polaris
e3sm_submodules/Omegabaseline-pfor both the baseline (Polarise3sm_submodules/Omega) and the PR buildPerformance related PRs: Please include a relevant PACE experiment link documenting performance before and after.
New tests:
Stealth Features