NorESMhub / BLOM

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

migrate HAMOCC config settings to run time namelist settings #281

Closed mvertens closed 8 months ago

mvertens commented 8 months ago

This PR builds on top of #280. Instead of using build time configuration settings for new HAMOCC use_XXX variables, namelist variables have been introduced instead. This allows HAMOCC to be configured at run time rather than at compile time. This PR was brought in as a separate pull request - but if desired be merged into #280.

The only files that are different with respect to #280 are

cime_config/buildcpp
cime_config/buildnml
cime_config/config_component.xml
cime_config/namelist_definition_blom.xml
drivers/mct/import_mct.F
drivers/nuopc/mod_nuopc_methods.F90
hamocc/hamocc4bcm.F90
hamocc/hamocc_init.F90
hamocc/mo_control_bgc.F90
hamocc/mo_param1_bgc.F90
JorgSchwinger commented 8 months ago

I think we should merge #280 first - it gets very confusing, because I see all changes at the same time (maybe that's just because I don't know how to switch this of?).

Anyway - this PR seems also to bring in the changes with the fluxes calculated in the mediator, and there are some changes related to mct, too. Is this intended?

mvertens commented 8 months ago

@JorgSchwinger - I agree. We need to bring in #280 first. The changes for fluxes in the mediator should not be there. I'll fix this once we bring #280 in. But I thought it would be good to have a run-time configurable for hamocc - since we removed the #ifdefs. That will make it much easier to use I think.

mvertens commented 8 months ago

@JorgSchwinger @TomasTorsvik @jmaerz - I'd really like some feedback as to what new tests I should add. I would propose at least the following:

1) Add turning on the namelist flags to .true. for all of the following: USE_CFC = .true. USE_WLIN = .true. USE_CISONEW = .true. USE_NATDIC = .true.

2) Do the above but turn on use_secbypass USE_CFC = .true. USE_WLIN = .true. USE_CISONEW = .true. USE_NATDIC = .true. USE_SEDBYPASS = .true.

I would do these as 5 day ERS tests at tn14

I also need to introduce tn21 for the development code - but I'll do that in a separate PR - since it will also need cice6 namelist changes.

JorgSchwinger commented 8 months ago

Looks good to me. But note that switching the CFCs and natDIC options on will mostly only test that it compiles correctly - if no transient compset is used then natDIC=DIC and CDFs=0 all the time. For a transient compset (e.g., NOIIAOC20TR) one would need to restart from e.g. 1948. That would be the most meaningful test, but this might get too complicated.

mvertens commented 8 months ago

@JorgSchwinger - thanks! I'm confused by the meaning of restart here. Can we do an initial run for NOIIAOC20TR - and point to 1948 files? Or do we need to start it up as a branch run and use the restart files? I think its worthwhile to continue testing this the right way.

JorgSchwinger commented 8 months ago

If we want to test natDIC and CFCs with "real" values we need to restart from 1948 files (or any other year over the period where atmospheric CFCs were > 0 and and atmospheric CO2 > 284)

JorgSchwinger commented 8 months ago

Caveat: for CO2 this requires that transient CO2 is available through the data atmosphere

mvertens commented 8 months ago

@JorgSchwinger - thanks for the clarifications. I might defer the more complicated testing for now and raise issues for their implementation.

JorgSchwinger commented 8 months ago

@mvertens, I think that makes sense - testing if natDIC and CFCs compiles and runs correctly is already very valuable

JorgSchwinger commented 8 months ago

Can we proceed with this PR? I would have two or three small PR that should go into release 1.4.0, but I think it would be better to merge this in first.

mvertens commented 8 months ago

I have not had a chance to add the new test yet. Could you please give me this afternoon to add the test and create the baselines - then I'm fine with proceeding. Sorry for the delay.

JorgSchwinger commented 8 months ago

Of course that's fine. Meanwhile I'll add some of the stuff already that will not interfere with this PR.

jmaerz commented 8 months ago

Dear @mvertens , many thanks for making the code more flexible to use! - could you also adjust the meson.build and meson_options.txt to reflect your code changes there as well - to further support the single column setup?

mvertens commented 8 months ago

@jmaerz - I am happy to work on the meson build. However, could we please chat today so that I can understand how to test this - both in the new and old system. Also - should that fix be added to this PR?

jmaerz commented 8 months ago

Hi @mvertens , sure, we can have a short conversation (while admittedly, I am also more a user of the meson.build system than developer). From what I know about it, it's just about cleaning out the previously used preprocessor flags for iHAMOCC (which are now configured via the config namelist). Wrt testing, I personally used to use it for the single column setup, which can be tested locally or on betzy and requires a limits file (generated from the namelist file), which I can provide (maybe need to update it to the latest developments). I am not sure, if there are other tests to perform than just check that such setup is still running. @TomasTorsvik: anything else to add on this?

mvertens commented 8 months ago

@jmaerz - how do I run a single column test? Is this documented?

jmaerz commented 8 months ago

Hi Mariana, it's described on the main page: https://github.com/NorESMhub/BLOM/ - maybe we chat quickly and then I'll provide the limits file, etc. afterward. If it is easier, I can also do the editing of the meson.build after the PR.

jmaerz commented 8 months ago

Hi Mariana, I just realize that some files experienced too long lines and compilation failed in master when testing with meson.... - so some hotfix is needed before the single column testing could work. I could do that also in preparation for #292 .

mvertens commented 8 months ago

A quick update:

jmaerz commented 8 months ago

@mvertens: with #293 meson compiles again as expected.

mvertens commented 8 months ago

I think at this point we should defer changes to the meson build until the next PR and try to get this one merged in if everyone is in agreement.

TomasTorsvik commented 8 months ago

@mvertens , @JorgSchwinger , @jmaerz This PR has been approved by all reviewers. Is it OK to merge, or are there any more checks that should pass first?

mvertens commented 8 months ago

@TomasTorsvik - I have verified that adding the new namelists to ocn_in does not change answers. From my point of view - this PR is ready to be merged.

jmaerz commented 8 months ago

Then go ahead @mvertens :+1: