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

New capability to have BLOM optionally use DMS and BROMO fluxes computed in mediator #259

Closed
wants to merge 45 commits into from

Conversation

mvertens
Copy link
Contributor

@mvertens mvertens commented Jun 16, 2023

Summary:

This PR does several things:

  1. Enables dms fluxes to be computed by the NUOPC CMEPS mediator
    (i.e. coupler) and sent back to blom. The PR should be totally
    backwards compatible with the original method where dms fluxes are
    computed in the HAMOCC code and sent to the coupler.
    NOTE: A new CMEPS tag XXX (this is under review now) is needed to turn on the following capabilities but should not be required to run BLOM.
    Obtaining dms fluxes from the mediatoris governed by a new CMEPS configuration variable, flds_dms_med, in
    the CMEPS nuopc.runconfig.
    If flds_dms_med is .true., then BLOM will send So_dms to the mediator and receive back Faox_dms.
    If flds_dms_med is .false., then BLOM will send Faoo_dms.

  2. NUOPC cap routines:

  • The blom NUOPC cap routines (ocn_comp_nuopc.F90 and mod_nuopc_methods.F90) have been updated in order to centralize the specification of the import/export fields (in mod_nuopc_methods.F90) and to implement the new logic whereby the method by which dms fluxes sent to the mediator are triggered by the NUOPC attribute
    flds_dms_med). Although the current implementation is based on the MOM6 cap - other caps in CESM have centralized the field specification in one module. The MOM6 cap should be updated as well.
  • The fldlist_add calls were moved from ocn_comp_nuopc.F90 tomod_nuopc_methods.F90 so that all field definitions would be in one file. As a result, any new field that is added to the import or export state only needs to be added in one module - mod_nuopc_methods.
  • The variables fco2_requested, fdms_requested and fbrf_requested are no longer needed. The logic to determine if the corresponding fieldsare sent/received from the mediator is based on the index being non-zero and the associated pointer being associated.
  • The index_xxxx variables are now module variables in mod_nuopc_methods.
  • As part of this a lot of run time write statements to the ocean log file documenting the optional fields that were being
    sent and received are now only written out once at the beginning of
    each run
  • Since mo_carbch and mo_param1_bgc are only compiled when HAMOCC is defined, the HAMOCC ifdef
    has been added to the use statements - but also added to blocks of code for consistency
  1. New regression testing
    The files buildnml and testlist_blom.xml have been changed in order to
    implement new regressions tests for the aux_blom test category.
    Buildnml has been modified so that daily history files are written for
    tests. This is needed in order for cprnc that is used by the CIME
    testing framework to work correctly:

  2. Default pe-layout
    The change to config_pes.xml resolves the issue with the model hanging
    in certain out of the box pe-layouts.

  3. A few notes:
    Nitrogen deposition is always sent from atm now (either CAM or DATM)

Testing:

ERS is an exact restart test. @JorgSchwinger - this verifies that restarts are bit-for-bit with the new implementatio
PFS is a performance test (20 days with no coupler history output) - this is used to determine if code changes to the system adversely effect performance
To see the full list of regression tests available in the CIME CCS system - see https://docs.google.com/document/d/1CZmIoTU6WGf-8EuuSRz98AAmp92IvSp16iE1Mx75af0/edit
This document should be used to update the regression tests we want to do for NorESM/BLOM

The following tests were run (more can be added as part of this PR)

  • ERS_Ld3.T62_tn14.NOINY.betzy_intel
    both with the new code and blom master at 7be980d - the two cases were bit-for-bit
  • ERS_Ld3.T62_tn14.NOINYOC.betzy_intel
    comparing the DMS flux on the blom restart files between the two cases showed close correspondence
    I'll add plots here once Betzy comes back up
  • PFS.T62_tn14.NOINYOC.betzy_intel
    This performance test showed no degradation in performance due to the new code

Below is the instantaneous difference after 2 days of simulation for NOINYOC at T62_tn14 in the flxdms as shown in the blom restart file. Spot checking show maximum differences on the order of 10%.
There are inherent lags that could account for this. In the original implementation there is a lag in terms of the atmospheric forcing used inside blom to compute the fluxes. In the new implementation there is a lag of the ocean concentration sent to the coupler.

diff nc
new nc
orig nc

@matsbn @JorgSchwinger - I am happy to do more testing and validation with this PR. I'm happy to set up a meeting to discuss the PR implementation.

@JorgSchwinger
Copy link
Contributor

A comment from the HAMOCC side: In the past years we have tried to develop a well defined interface between BLOM(CESM) and HAMOCC. Everything that interfaces with BLOM happens in hamocc_init and hamocc_step. The "core hamocc", which is everything below the call to hamocc4bcm in hamocc_step should be independent of any BLOM (or CESM) code.

With this background, but also since flxdms is anyway passed to hamocc4bcm as an argument, I would prefer to not include cesm modules in carchm, but instead handle everything in hamocc4bcm, where flxdms would be either used to update the ocean concentration if it was sent from the mediator, or would be assigned the flux calculated in carchm, depending on the setting of do_dmsflx_med. I think this would be more transparent.

I'm happy to discuss this further (I will be in Oslo at the KeyClim meeting this week). I can also try to push my suggestion to this branch, although I have to admit that this stretches my github skills to the limit... But I'll find out how to do this if you think this is a good idea.

@JorgSchwinger
Copy link
Contributor

And the differences seen above seem plausible to me, given the different time-lags involved.

@mvertens
Copy link
Contributor Author

@JorgSchwinger - thanks so much for the input. Being consistent with the current code architecture makes total sense and I'm happy to implement your suggestions. What I did was just a first cut for to determine the feasibility of getting the fluxes from the mediator with the minimum invasion to the code base. Since I have answers to compare with now - it should be fairly straightforward to do this and I'm happy to take it on. It has also been a good way for me to become more familiar with the BLOM code.
It would be great to discuss more at the KeyClim meeting this week. See you tomorrow.

@mvertens mvertens changed the title New capability to have BLOM optionally use DMS fluxes computed in mediator New capability to have BLOM optionally use DMS and BROMO fluxes computed in mediator Jun 28, 2023
@JorgSchwinger
Copy link
Contributor

Looking through theses changes I realize that if we have bromoform as a default, we would need an swa-climatology file for every BLOM grid/setup. This is a consequence I didn't realize when we discussed this in Oslo (sorry about that), but I think it would be good to avoid this.

Could you remind me, what was the issue with the #ifdef BROMO? I see there is still #ifdef HAMOCCS in the code, so I am wondering if it would be impossible to keep the #ifdef BROMOs for now?

Sorry for not bringing this up earlier. Also, I'll be unavailable for the next 2.5 weeks.

@mvertens
Copy link
Contributor Author

mvertens commented Jul 1, 2023

@JorgSchwinger - I realized that as well and was wondering if this would impact the implementation. I can try to come up with a scheme that would allow the #ifdefs to be included. What would happen is that if you asked for the coupling of bromoform from the NUOPC driver - then there would have to be a check in the NUOPC cap that indeed BROMO was also defined - otherwise the model should abort. I think that should do it. I'll try to implement this and we can talk when you get back.

Copy link
Contributor

@JorgSchwinger JorgSchwinger left a comment

Choose a reason for hiding this comment

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

This looks fine to me (I mostly reviewed the HAMOCC side though). There seem to be some changes touching this PR in #264?

! Set the logical flag do_bgc_aofluxes to false in mo_control_bgc since
! for nuopc/cmeps the dms and bromo fluxes will be computed in the mediator
do_bgc_aofluxes = .false.
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any possibility to let the user make this choice? For example, if this could be made a namelist variable (with defalt value .false.) it would be easy to activate the old HAMOCC scheme. But I fear this information is needed before the BLOM/HAMOCC namelists are read in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it would be best to have this as a namelist choice, but I think this is better to introduce in connection with PR #263 , so not to have namelist changes introduced multiple times.

@mvertens
Copy link
Contributor Author

mvertens commented Aug 2, 2023

Note that this PR should be merged after the namelist refactor PR is approved and merged since it builds on top of it.

@TomasTorsvik
Copy link
Contributor

Note that this PR should be merged after the namelist refactor PR is approved and merged since it builds on top of it.

@mvertens - Thanks for clarification. I have set the status for this PR as "blocked" pending approval of PR #263 .

Copy link
Contributor

@TomasTorsvik TomasTorsvik left a comment

Choose a reason for hiding this comment

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

I'm fine with theses code changes. However, I don't understand why the PR should remove the CI workflow for BLOM.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
! Set the logical flag do_bgc_aofluxes to false in mo_control_bgc since
! for nuopc/cmeps the dms and bromo fluxes will be computed in the mediator
do_bgc_aofluxes = .false.
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it would be best to have this as a namelist choice, but I think this is better to introduce in connection with PR #263 , so not to have namelist changes introduced multiple times.

@mvertens
Copy link
Contributor Author

mvertens commented Aug 2, 2023

@JorgSchwinger @jmaerz - PR #263 should come in before this PR. This PR is built on top of it.

@mvertens
Copy link
Contributor Author

mvertens commented Aug 2, 2023

@TomasTorsvik - the variable do_bgc_aofluxes is only introduced in #259 and not in #263. Its unfortunate that PR numbers are in reverse order from how they should be merged. So to introduce do_bgc_aofluxes as a namelist variable - I would have to do this in #259 which is built now on top of #263.

@JorgSchwinger
Copy link
Contributor

But in principle, it would be possible to have do_bgc_aofluxes in the namelist, i.e. there is no need to have it defined before the namelists are read in? Then this could be done in a separate PR after everything has been tested and merged.

@mvertens
Copy link
Contributor Author

mvertens commented Aug 2, 2023

@JorgSchwinger - I agree that this would be the right sequence of bringing this in as a namelist.

@mvertens
Copy link
Contributor Author

mvertens commented Nov 9, 2023

At this point I'd like to close this and reopen it later if we feel this is still the right way to go.

@mvertens mvertens closed this Nov 9, 2023
@mvertens mvertens deleted the feature/dms_flux_frmed branch May 15, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants