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

Add MARBL to MOM6 #157

Merged
merged 205 commits into from
Aug 2, 2024
Merged

Conversation

mnlevy1981
Copy link
Collaborator

I'm working on adding a new tracer module that calls MARBL, and I've reached a point where it might be useful for other people (e.g. @ashao) to look at the mods I'm making. There will also be a PR to ESCOMP/MOM_Interface necessary to get this running through CESM, and I'll link to that PR once it has been made.

mnlevy1981 added 30 commits May 16, 2019 15:54
Copied dye_example.F90, renamed all public routines. This module does not
actually tie into MARBL at this time.
Adding USE_MARBL_TRACERS = True to override file turns on MARBL tracers. At
this point, we call marbl_instances%init and register all 32 tracers but don't
do anything else (so they are initialized to 0 and there is no source term for
advecting them yet)
CESM will use MARBL via manage_externals, but other systems may need to bring
it in via submodules
I think this is just needed for TravisCI, since it doesn't know to look for
code in pkg/
This looks like it was removed from everywhere else when I merged in the latest
dev/ncar branch
Created new configure_MARBL_tracers() subroutine to be called from
call_tracer_register() [between get_param() calls and register_* calls]
Also uses put_settings() to update the MARBL settings before initialization
(tested by setting ciso_on = .true. via user_nl_marbl)
marbl_instance%StatusLog is written after the call to init and in
marbl_tracers_end (which is now called from tracer_flow_control_end())
Failed a TravisCI test due to missing documentation
Also added a placeholder for parsing the MARBL timing information
Every call to print_marbl_log() is followed by a call to %erase()
Instead of passively advecting 0s, the MARBL_tracers module now correctly
initializes each tracer (but doesn't compute any source-sink terms yet)
Went from 0a806cf to 479f914
call the function from register_MARBL_tracers() rather than
MOM_tracer_flow_control.
MARBL will be brought in to CESM via manage_externals, and we will use
-DUSE_MARBL_TRACERS to build with access to MARBL.
Now that it comes in from manage_externals, we want to ignore it
Can build MOM without pkg/MARBL, but if USE_MARBL_TRACERS is True in the param
file and the build does not include -D_USE_MARBL_TRACERS then the model aborts.
Will allow MARBL diagnostics to be added to history files.
And, from that routine, call marbl_instances%surface_flux_compute().

Note that forcings, surface tracers, and surface fluxes are all zeroed out in
this commit. I'd like to get diagnostics posted in the next commit, and then I
can start updating tracer surface values, saving saved state, and looking into
how to read forcing fields.
Looks like column_physics() is the better place for this call
calls post_data (note that created a temporary data structure to hold both the
diagnostic id and a buffer to fill column-by-column as MARBL runs)
During configuration, set indices for each of the surface flux forcings so that
each forcing can be set to a different value in column_physics(); all are set
to zero except u10_sqr (2.5e5), atmpress (1), and xco2 / xco2_alt_co2 (284.7)
With solo_driver, the following runs are all bit-for-bit with non-scaled runs:

C_RESCALE_POWER = 10
H_RESCALE_POWER = 10
L_RESCALE_POWER = 10
S_RESCALE_POWER = 10
T_RESCALE_POWER = 10
Z_RESCALE_POWER = 10
Should pass doxygen test again
The function is meant to help copy fields from the ice_ocean_boundary_type
(which is in physical units in all the caps) to the forcing_type (which wants
scaled units). So the solo_driver should NOT scale the dust, black carbon, or
NDEP inputs from data_override, and instead that scaling should happen in
MARBL_forcing_mod.F90
applyTracerBoundaryFluxesInOut expects in_flux_optional in units of conc H, and
we were passing conc m T/s. Since riv_flux_loc is now conc H, I also added a
debug-gated hchksum on it.
The dimensional scaling tests fail if the MARBL tracer concentrations are very
very small (O(1e-300)); this can be avoided by setting the minimum tracer value
to be 1e-100 instead of 0. We don't want to do this for production runs,
though, so the default for this parameter is still 0.
Sa_co2prog and Sa_co2diag should not be area corrected (they are states) but
Faoo_fco2_ocn should be (it's a flux)
When calling marbl_instance%init(), we should tell MARBL that MOM6 doesn't have
the global operators that MARBL expects (global sums / running means) so we get
the appropriate error message when trying to run with ladjust_bury_coeff = True
Updated ice_ocn_bnd_type_chksum() in the NUOPC cap, though I don't think this
function is ever called
I had created CESM_INPUTDATA as a parameter to point to my work directory, but
it is no longer necessary because INPUTDATA points to the CESM input data
repository and I've moved necessary files there
@mnlevy1981
Copy link
Collaborator Author

@marshallward -- we (@alperaltuntas, @gustavo-marques, @klindsay28, and I) are starting the review process of this PR, and had a couple of questions that I hope you can answer:

  1. I created src/tracer/tracer_forcing_utils.F90, a module that extends the interface to time_interp_external to allow things like "use the first time level of a forcing file until model year X" or "after model year Y, use the last time level of a forcing file." I put it in src/tracer/ thinking it would be useful for both the MARBL tracer module and the CFCs but it's not really tracer-specific. Do you have a recommendation for location and / or name? We were thinking maybe src/framework or src/core.
  2. I made modifications to src/initialization/MOM_tracer_initialization_from_Z.F90 to make the default values of Z_INIT_ALE_REMAPPING and Z_INIT_REMAPPING_SCHEME match what is in MOM_state_initialization.F90. Should I be using do_not_log=.true. instead? (There are also get_param() calls in my MARBL-specific modules to things like INPUTDIR where I opted to repeat the existing default rather than use do_not_log.)

I'd welcome general feedback here as well, though we're still going through the detailed code review so it might make sense to wait until I've had a chance to fix the issues raised from that (or wait until we try to get this to main on mom-ocean/MOM6)

@marshallward
Copy link

  1. I created src/tracer/tracer_forcing_utils.F90, a module that extends the interface to time_interp_external to allow things like "use the first time level of a forcing file until model year X" or "after model year Y, use the last time level of a forcing file." I put it in src/tracer/ thinking it would be useful for both the MARBL tracer module and the CFCs but it's not really tracer-specific. Do you have a recommendation for location and / or name? We were thinking maybe src/framework or src/core.

If its purpose is to provide some fine-grain control over time_interp_external(), then I would say it belongs in src/framework, possibly even move the content to MOM_interpolate.F90. I don't know exactly how the new function connects to time_interp_external, but I could see adding the new type as either an optional argument or as an alternative to the existing time_interp_external arguments (although maybe I'm being too creative here).

In any case, I agree it looks very general purpose and potentially useful to others, and it would be good to move outside of src/tracer, although there might need to be some discussion about the implementation.

  1. I made modifications to src/initialization/MOM_tracer_initialization_from_Z.F90 to make the default values of Z_INIT_ALE_REMAPPING and Z_INIT_REMAPPING_SCHEME match what is in MOM_state_initialization.F90. Should I be using do_not_log=.true. instead? (There are also get_param() calls in my MARBL-specific modules to things like INPUTDIR where I opted to repeat the existing default rather than use do_not_log.)

do_not_log is generally only used when the parameter has already been read, and we don't want it appear twice in MOM_parameter_doc.*. I suppose it might need a just_read argument similar to MOM_temp_salt_initialize_from_Z, but it's hard for me to tell from just looking. I suppose if it appears twice in MOM_parameter_doc.all, then yes, you need do_not_log=.true. or something similar 😄 .

-- cleaned up a lot of comments and whitespace
-- used source argument in more allocate statements, and deallocated more
   arrays
-- 3D diags now have zl:mean in cell_methods attribute
-- marbl_instances%domain%kmt is set once (during initialization)
@mnlevy1981
Copy link
Collaborator Author

mnlevy1981 commented Jul 12, 2024

21529b9 addressed most of the concerns from code review. Still remaining

  • Call MARBL_tracer_stocks from MOM_tracer_flow_control:call_tracer_stocks()
  • Can of worms in MOM_tracer_flow_control:get_chl_from_model: need better logic for when to get chlorophyll from generic tracers vs getting it from MARBL_tracers (perhaps a CS flag get_chl_from_generic_tracers that defaults to True but gets set to False if MARBL is running with base biotic tracers?)
  • I also haven't made any changes based on @marshallward's suggestions in Add MARBL to MOM6 #157 (comment)

I'll note that I ignored two suggestions from the review:

  1. I wrote down that I should change FESEDFLUX units from conc m/s -> conc Z/T, but this is a forcing file that never gets converted from physical units to model units
  2. There was a comment in MOM_forcing_type.F90 (!These fields should only be allocated when USE_MARBL is activated.) that I was going to remove, but in the context of the other calls to myAlloc() I think we want to leave it in.

Next steps:

  • finish the checklist above
  • update MARBL Support ESCOMP/MOM_interface#92 based on review so I can test solo_driver
  • merge latest dev/ncar here and main in MOM_interface, and test in latest CESM tag

If MARBL is not configured to provide the base biotic tracers, then it will not
be able to provide chlorophyll. In that case, if CHL_FROM_FILE=False, MOM6
needs to get chlorophyll from the generic tracers.
To make these subroutines more accessible, they were moved out of src/tracer/
and made available through MOM_interpolate
@mnlevy1981
Copy link
Collaborator Author

As of 97c0917 I've addressed all the concerns raised in code review, and only see expected test failures (MacOS-related). I'll leave this in draft mode while addressing the companion PR at MOM_interface.

@mnlevy1981
Copy link
Collaborator Author

(I may also need to merge in the latest dev/ncar; I wanted to address all the code review comments in my cesm2_3_alpha17f sandbox before moving to the cesm3_0 alpha / beta tags)

If variable was described in POP comment, I copied the comment over. Otherwise
I came up with a description on my own.
time_interp_external() does not update halo regions, so running CESM with
DEBUG=TRUE was triggering some overflows from uninitialized memory. Intead of
copying the entire array, we now loop through (is:ie,js:je) when accessing an
array returned from time_interp_external()
@mnlevy1981 mnlevy1981 marked this pull request as ready for review August 1, 2024 21:06
Most use cases don't include restoring for MARBL tracers, but when that feature
is enabled and the time scale is read from a file the user can specify what
variable to read from the netCDF file (default is I_TAU to match naming
convention in MOM6, but some test cases are based on POP files and will need to
read RTAU)
@gustavo-marques gustavo-marques changed the title [Work in Progress] Add MARBL to MOM6 Add MARBL to MOM6 Aug 2, 2024
@gustavo-marques gustavo-marques merged commit 8b9ba97 into NCAR:dev/ncar Aug 2, 2024
8 of 10 checks passed
call ESMF_FieldGet(lfield, farrayPtr=fldptr2d, rc=lrc)
if (ChkErr(rc,__LINE__,u_FILE_u)) return

if (.not. associated(fldptr)) allocate(fldptr(size(fldptr2d,2)))
Copy link
Member

Choose a reason for hiding this comment

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

This line causes a memory leak. Any fix suggestions @mnlevy1981 ?

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.

7 participants