Skip to content
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

MARBL Support #92

Merged
merged 153 commits into from
Aug 5, 2024
Merged

MARBL Support #92

merged 153 commits into from
Aug 5, 2024

Conversation

mnlevy1981
Copy link
Collaborator

When NCAR/MOM6#157 is merged, this PR will be required to build CESM + MARBL. I think this branch is fully functional, but there may be a few small tweaks to things like default parameters or diag table settings. Replaces #75 to reflect the master -> main renaming.

I'll take it out of draft status once the MOM6 PR is ready for review.

This commit uncomments out code so that MARBL is built out of pkg/MARBL/src
This creates OCN_BGC_CONFIG in env_run.xml; as long as OCN_BGC_CONFIG != none
(the default value), USE_MARBL_TRACERS = True.
Hardcodes the names of 32 tracers to include in diag_table if running with
MARBL enabled. Note that eventually this will have to be replaced with
something more robust that relies on MARBL to report the names of the active
tracers... as it stands, enabling CISO will not increase output at all, and
running with cocco will not work (spCaCO3 is not a valid tracer name in that
configuration)
Default to 432 tasks on cheyenne instead of 144 when running with MARBL's
tracers.
Uses the CESM_x1 default values, allows override via user_nl_marbl
Updates MOM_input.json and diag_table.json
I missed a change in the default diag_table
Bring it in as a typical external instead
Eventually MARBL will only be built if a BGC-enabled compset is specified, so
it makes sense to only look for pkg/MARBL and add it to paths if it will be
needed. At this point it is in an "if True:" block, but down the line the
condition will change.
Adding -D_USE_MARBL_TRACERS to the build requires a CIME mod:

$ git diff
diff --git a/config/cesm/machines/config_compilers.xml
b/config/cesm/machines/config_compilers.xml
index fca45de02..c3cb51a2c 100644
--- a/config/cesm/machines/config_compilers.xml
+++ b/config/cesm/machines/config_compilers.xml
@@ -80,7 +80,7 @@ using a fortran linker.
   </INCLDIR>
   <FFLAGS>
     <append MODEL="fv3gfs"> $(FC_AUTO_R8) </append>
-    <append MODEL="mom"> $(FC_AUTO_R8) -Duse_LARGEFILE</append>
+    <append MODEL="mom"> $(FC_AUTO_R8) -Duse_LARGEFILE
-D_USE_MARBL_TRACERS</append>
   </FFLAGS>
   <SUPPORTS_CXX>FALSE</SUPPORTS_CXX>
 </compiler>

This is likely where changes to only enable the build CPP in some compsets will
need to go, and it will be handy to have a comment pointing to the file.
Added ECOSYS_ATMPRESS and ECOSYS_IFRAC to hm_bgc history file to make sure the
correct values were coming in from the coupler. Also increased NTASKS_OCN to
864 and set up pe layout for G1850MOMECO compset.
Stream is daily through test suite.

Also added PH to the stream (regardless of whether run is a test)
If MARBL is enabled (OCN_BGC_CONFIG anything but "none"), then diag_table comes
from both diag_table.json and diag_table_MARBL.json; the latter is copied to
Buildconf/momconf and then read in, but future commits will generate the json
file in place in Buildconf/
With a new case, it turned out buildnml was trying to copy a file to
Buildconf/momconf before that directory was created
File is created by buildnml; next step is to turn it into a JSON file
1. differentiate between r05 and JRA025 runoff grids
2. update the JRA025 runoff grid to include data at Jan 1, 1900 and Jan 1, 2001
(necessary for having MOM6's time interpolation use Jan 1, 1900 as the forcing
date when not a transient case)
* Replaced MOM6%MARBL with MOM6%MARBL-BIO and also added MOM6%MARBL-ABIO to
  possible compset longnames.
* PE layouts only get altered for MOM6%MARBL-BIO compsets, since the ABIO_DIC
  module doesn't need extra cores
* Added a new MARBL_TRACER_OPTS variable to env_run.xml that may contain
  INCLUDE_BASE_BIO, INCLUDE_ABIO_DIC, or both.
  - diag_table and default parameter settings check for these flags

NOTE: MARBL doesn't currently have a mechanism to let the GCM set base_bio_on
or abio_dic_on in marbl_in directly; will need to improve the MARBL scripts and
then have a second commit to make sure the those variables reflect what the
user wants. Currently the defaults are base_bio_on=T and abio_dic_on=F so the
following holds:

i. MOM6%MARBL-BIO works as expected
ii. For MOM6%MARBL-BIO%MARBL-ABIO, user needs to set abio_dic_on=T in
    user_nl_marbl
iii. For MOM6%MARBL-ABIO, user needs to set base_bio_on=F and abio_dic_on=T in
     user_nl_marbl
MARBL_TRACER_OPTS now sets values in marbl_in correctly
Use BASE_BIO_ON=TRUE instead of INCLUDE_BASE_BIO and ABIO_DIC_ON=TRUE instead
of INCLUDE_ABIO_DIC. Also updated the way we get the base_bio_on and
abio_dic_on arguments for MARBL_settings_for_MOM(); users can specific
ABIO_DIC_ON=TRUE or ABIO_DIC_ON=FALSE but any other values (including True,
true, T, False, false, or F) will fail. We need to be so strict because
ParamGen is looking specifically for "ABIO_DIC_ON=TRUE" when setting values for
MOM_input.
To support both base bio and abio, we need marbl0.46.1
Still using 20 nodes for MOM6%MARBL but now it's 21 total nodes instead of 22
I didn't quite stick the landing on the last merge, and left one conflict
marker in that was causing issues with xmllint. While I was fixing that, I also
moved the aux_mom_MARBL from cheyenne to derecho and switched from CORE forcing
to JRA compsets
This PR makes sure check_input_data looks for MARBL input files when running
with MARBL tracers
Instead of hard-coding packing=1 (double precision), use "1 if $TEST else 2"
like what is found in diag_table.yaml
Updated how diag_table_MARBL.json was generated to include an OCN_DIAG_MODE !=
"none" test (like physics stream files)
This MARBL tag introduces the idea of diag_mode to help select which
diagnostics are included in the output. Support in MOM requires a new
env_run.xml variable (MARBL_DIAG_MODE), which gets passed from buildnml to
several of the MARBL_scripts.

Note that the MARBL diagnostics are now output in single precision unless
MARBL_DIAG_MODE == "test_suite" (in which case it gets bumped up to double
precision).
Having so available in h.bgc.native instead of relying on both that stream and
h.native makes things a little easier for analysis. I also added thetao to the
stream when diag_mode >= 'full'
@mnlevy1981 mnlevy1981 marked this pull request as ready for review June 5, 2024 16:25
The check for the riverine input file for MARBL was not being run in fully
coupled cases because I had the logic of when to look for the dataset mapped
from r05 to tx23v2 set wrong in the yaml / json files
Comment on lines +10 to +12
<!-- match filenames of the form
h.bgc.*[._optional instance number].nc -->
<hist_file_extension>h\.bgc\..*?.?[_\d+]+.nc$</hist_file_extension>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It turns out this block is necessary -- without it, h.bgc.native, h.bgc.daily, and other h.bgc.* streams are not archived. I wonder if one fix would be to change lines 5 and 6:

    <!-- match filenames of the form
-         h[.optional stream name][._optional instance number].(date string).nc[.optional tile number] -->
-    <hist_file_extension>h(\.\w+)?(\._\d*)?\.[-\d+]+\.nc(\.\d*)?$</hist_file_extension>
+         h.stream_name.[.optional stream name][._optional instance number].(date string).nc[.optional tile number] -->
+    <hist_file_extension>h\.\w+(\.\w+)?(\._\d*)?\.[-\d+]+\.nc(\.\d*)?$</hist_file_extension>

We don't generate a mom6.h, do we? I'm going to leave this file as-is, but we can revisit it after it's merged -- I don't want to get side-tracked going down a rabbit-hole of testing various possible changes when this works and removing it doesn't.

1. cleaned up config_pes.xml
   * removed gx1v7 grid and hobart machine
   * Use more nodes for MARBL-BIO, but ABIO-only runs on same config as
     non-MARBL
2. dropped box_atm_co2 option from OCN_CO2_TYPE
3. cleaned up MARBL_CONFIG default values
4. cleaned up config_compsets.xml
   * new ABIO compset
   * removed outdated compsets that I accidentally kept in previous merge
5. Added tests for MARBL-ABIO, and removed Vnuopc test. Also test on more G
   compsets and fewer C compsets
6. Fixed some logic in buildnml, and moved imports to top of file
7. MOM_input.yaml:
   * consistent formatting for descriptions
   * added actual descriptions instead of "TODO: add description"
   * Added ATM_ALT_CO2_OPT
   * ICE_NCAT and USE_ICE_CATEGORIES only set for BASE_BIO_ON=TRUE
   * Set MIX_BOUNDARY_TRACER_ALE=True for all configurations (otherwise turning
     on MARBL affects IAGE)
8. Drop tx0.66v1 default values from all parameter files
9. Moved D14C forcing files to grid_indep/
10. Lots of "ecosys" -> "MARBL" clean-up
    * noteably, test is now no_MARBL instead of no_ecosys
    * generate MARBL_diagnostics file instead of ecosys_diagnostics ahead of
      diag_table
@mnlevy1981
Copy link
Collaborator Author

mnlevy1981 commented Jul 17, 2024

dbe3d4e got most of the easy-to-implement suggestions from the code reviews, and then c0c8d88 ran black on the scripts that I had created to help buildnml call python code from the MARBL repo. Still remaining

  • Investigate whether my FMS changes are actually necessary, or if they can go in input_nml.yaml instead
  • Add G1850MOMMARBL tests to prealpha and prebeta (SMS in former, ERS in latter... this should've happened in dbe3d4e!)
  • Merge in latest main. The switch from manage_externals to git fleximod is going to be a little intrusive (we'll move MARBL from ${MOM_interface}/MOM6/pkg/MARBL to ${MOM_interface}/externals/MARBL since ${MOM_interface}/MOM6 isn't actually part of MOM_interface), and then I'll need to update buildlib and the stand-alone examples.

I did not look into replacing standalone/examples with a sparse checkout of MOM6-examples; that seems like something to tackle after this PR is merged in because we'll need to decide if we want to support our own fork or use the same code as GFDL before adding single_column_MARBL/ to that repo. I also didn't look at having create_newcase create an empty user_nl_marbl file (that's definitely a feature I'd like to have in the future, but it's not necessary in the first pass), nor did I add default values to all the MARBL parameters because we don't want them in MOM_input unless we are running MARBL (maybe I'm misinterpreting my notes on that suggestion?).

Also updated MOM6 to use dev/ncar_240802
Added the following two tests:

prealpha: SMS.TL319_t232.G1850MARBL_JRA.derecho_intel
prebeta: ERS.TL319_t232.G1850MARBL_JRA.derecho_intel

The former is also in aux_mom, and the latter is also in aux_mom_MARBL (those
are not new additions)
@mnlevy1981
Copy link
Collaborator Author

Investigate whether my FMS changes are actually necessary, or if they can go in input_nml.yaml instead

I do need to open an FMS PR to handle two changes

diff --git a/affinity/affinity.c b/affinity/affinity.c
index fff6622f..10b5a753 100644
--- a/affinity/affinity.c
+++ b/affinity/affinity.c
@@ -48,7 +48,7 @@
  * gettid function for systems that do not have this function (e.g. on Mac OS.)
  */
 #ifndef HAVE_GETTID
-static pid_t gettid(void)
+pid_t gettid(void)
 {
 #if defined(__APPLE__)
   uint64_t tid64;
diff --git a/diag_manager/diag_data.F90 b/diag_manager/diag_data.F90
index a1f59470..6791f91c 100644
--- a/diag_manager/diag_data.F90
+++ b/diag_manager/diag_data.F90
@@ -64,7 +64,7 @@ MODULE diag_data_mod
   PUBLIC

   ! Specify storage limits for fixed size tables used for pointers, etc.
-  INTEGER, PARAMETER :: MAX_FIELDS_PER_FILE = 300 !< Maximum number of fields per file.
+  INTEGER, PARAMETER :: MAX_FIELDS_PER_FILE = 500 !< Maximum number of fields per file.
   INTEGER, PARAMETER :: DIAG_OTHER = 0
   INTEGER, PARAMETER :: DIAG_OCEAN = 1
   INTEGER, PARAMETER :: DIAG_ALL   = 2

The former is necessary for building MOM6 with solo_driver instead of the NUOPC cap, and the latter is necessary because the test suite includes every possible MARBL output field.

@mnlevy1981
Copy link
Collaborator Author

I do need to open an FMS PR

That PR is ESCOMP/FMS#2

@gustavo-marques gustavo-marques merged commit 7cbb891 into ESCOMP:main Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants