NorESMhub / BLOM

Bergen Layered Ocean Model
GNU Lesser General Public License v3.0
16 stars 25 forks source link

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

Closed mvertens closed 7 months ago

mvertens commented 1 year ago

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:

3) 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:

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

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

Testing:

ERSis 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)

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 commented 1 year ago

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 commented 1 year ago

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

mvertens commented 1 year ago

@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.

JorgSchwinger commented 1 year ago

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 commented 1 year ago

@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.

mvertens commented 11 months ago

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

TomasTorsvik commented 11 months ago

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 .

mvertens commented 11 months ago

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

mvertens commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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

mvertens commented 7 months ago

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