migrate HAMOCC config settings to run time namelist settings #281

mvertens · 2023-10-15T18:07:01Z

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 · 2023-10-16T07:12:46Z

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 · 2023-10-16T07:20:20Z

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

JorgSchwinger · 2023-10-17T13:28:52Z

drivers/mct/import_mct.F

-                  atmco2_da(i,j,l2ci) =
-     .               x2o_o%rAttr(index_x2o_Sa_co2prog,n)
-               endif
+      if (ocn_co2_type == 'prognostic') then


This would now need a #ifdef HAMOCC, too since ocn_co2_type will be undefined otherwise

Thanks. I will be testing this PR with mct as well.

JorgSchwinger · 2023-10-18T08:54:40Z

cime_config/buildnml

+        if pg_blom.get_value('use_cisonew') == ".true.":
+            if pg_blom.bet_value("use_SEDBYPASS") == ".false.":
+                expect(False,
+                       "HAMOCC C-isotopes currently not supported in the sediment module. Set use_SEDBYPASS to .true.")


This should be removed now, since C-isotopes with sediment is technically supported now.

mvertens · 2023-10-18T09:08:05Z

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

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.
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 · 2023-10-18T09:18:21Z

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 · 2023-10-18T09:27:38Z

@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 · 2023-10-18T09:41:20Z

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 · 2023-10-18T09:42:35Z

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

TomasTorsvik · 2023-10-18T16:24:00Z

cime_config/buildcpp

-                    # HAMOCC bromoform scheme currently only supported on the tnx1v4 grid 
-                    # (no swa-climatology has been created for other grid configurations)"


This is perhaps an important comment to preserve, i.e. why only tnx1v4 gid is supported.

@TomasTorsvik - use_BROMO is now a namelist variable - not a CPP variable -
this is why I removed it from buildcpp. I have now added the comment to namelist_definition_blom.xml.

TomasTorsvik · 2023-10-18T20:13:46Z

cime_config/namelist_definition_blom.xml

+    <desc>HAMOCC debug mode flag</desc>
+  </entry>
+
+  <entry id="use_BROMO">


Suggested change

<entry id="use_BROMO">





<entry id="use_BROMO">

Maybe something like this?

Exactly. Thanks!

JorgSchwinger · 2023-10-19T11:28:46Z

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 · 2023-10-19T11:30:00Z

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 · 2023-10-19T11:53:28Z

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

JorgSchwinger · 2023-10-20T06:04:22Z

cime_config/namelist_definition_blom.xml

@mvertens, @TomasTorsvik, @jmaerz there are some things I came across during my own testing yesterday:

for some HAMOCC entries a is_input_data="yes" flag is missing (inidic, inialk,inipo3,inioxy,inino3,ndepfile,fedepfile,rivinfile,swaclimfile).

the value for ndepfile isn't generated correctly if blom_ndep_scenario=ssp* i.e. any of the SSPs (I guess $BLOM_NDEP_SCENARIO doesn't exist any longer?)

do_sedspinup and sedspin_ncyc should have a modify_via_xml flag

I think the C-iso option should stay as an option in env_run, and get the modify_via_xml flag, too

Maybe this could also be quickly fixed with this PR?

@JorgSchwinger - thanks for catching these! I will fix them in this PR.

Thanks for finding these. I've put in the fixes and am regenerating tests and baselines now.

jmaerz · 2023-10-20T09:49:36Z

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 · 2023-10-20T12:10:04Z

@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 · 2023-10-20T12:27:32Z

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 · 2023-10-20T12:39:45Z

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

jmaerz · 2023-10-20T12:42:09Z

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 · 2023-10-20T13:26:32Z

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 · 2023-10-20T13:35:47Z

A quick update:

I have incorporated fixes for the problems that were outlined by @JorgSchwinger.
I have generated a new baseline in /cluster/shared/noresm/noresm_baselines/aux_blom_noresm_20102023/.
I verified that answers do not change for those cases where we expect bfb.
I have two new tests that use the namelists settings for hamocc.
I will wait to work on the meson.build once fixes from @jmaerz are in.

jmaerz · 2023-10-20T13:46:41Z

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

mvertens · 2023-10-22T18:31:48Z

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.

jmaerz · 2023-10-23T10:03:23Z

cime_config/namelist_definition_blom.xml

+  </entry>
+
+  <!-- hamocc configuration -->
+


Dear @mvertens , if I understand the approach correct, the entry ids for use_Agg and use_BOXATM are missing.

@jmaerz - thanks for catching this. I think we need to add two more tests for use_BOXATM and use_AGG.
I'll do that today.

So in looking at the original buildcpp - BOXATM and AGG were never defined - and I only introduced namelists that corresponded to those variables. That was clearly an oversight on my part. Do we want to test turning these on with hamocc given that they were not defined as part of NorESM configurations before?

Hi Mariana, my suspicion is that both cases are not well supported, but I would hope that particularly AGG isn't broken (it was used the last time in a study by @JorgSchwinger). AGG should work within a coupled NorESM setup, while I have limited idea about BOXATM - I would need to look into it - I would suggest to skip the testing for BOXATM now, but I feel it would be good to enable switching it on via the xml. I hope to get through your changes by the end of the day.

@jmaerz - use_BOXATM and use_AGG are only turned on via user_nl_blom right now - no longer throught the xml since they are no longer CPP variables. I will test that use_AGG works and add a test today.

Hi @mvertens , I just realize that use_FB_BGC_OCE might also be missing?

BOXATM, AGG, FB_BGC_OCN all are options that have not been used quite some time. So currently, these are only for expert users who can activate these options via namelist switches if they know what they are doing.

How much it makes sense to include tests for these options I don't know, I wouldn't see this as a priority.

@JorgSchwinger @jmaerz - I have added missing namelists use_boxatm, use_agg and use_fb_bgc_oce to the new namelist group config_bgc. Given the last statement by @JorgSchwinger - can we test these options after this PR and add appropriate tests if everyone agrees? Otherwise - I'm happy to test it in this PR.

Hi @mvertens , from my side, I am totally fine to proceed as you suggested. The testing feature is different from the nml configuration.

jmaerz

Dear @mvertens , apart from the comments that I had, it looks good to me.

TomasTorsvik · 2023-10-24T09:23:57Z

@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 · 2023-10-24T11:38:50Z

@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 · 2023-10-24T11:43:31Z

Then go ahead @mvertens 👍

mvertens added 6 commits October 15, 2023 15:17

removed all hamocc cpp variables - made them namelist settings

9df579a

removed buildnml.csh

b8bb56a

removed incorrect write statement that was re-introduced

c0a4824

added back in loop for burial variables that was omitted in this PR

e7a9ef9

more fixes

eb003cc

removed hamocc config variables for run time namelist settings

36e488d

mvertens requested review from matsbn, JorgSchwinger and TomasTorsvik October 15, 2023 18:07

update to feature/refactor_ifdefs2

9768273

re-introduced files that should not have been deleted

ce12ae5

This was referenced Oct 16, 2023

remove most hamocc #ifdefs in favorite of new logicals #264

Closed

remove #ifdefs from HAMOCC #280

Merged

mvertens added 2 commits October 17, 2023 11:19

updates to hamocc master

78e347c

Merge branch 'master' into feature/refactor_ifdefs3

fa9ef0f

JorgSchwinger approved these changes Oct 17, 2023

View reviewed changes

mvertens mentioned this pull request Oct 18, 2023

NorESM is no longer bfb identical after merging PR-280 #285

Closed

JorgSchwinger approved these changes Oct 18, 2023

View reviewed changes

merged to blom master

e54e67b

fixed problems that arose in merge

133564d

updates for mct cap

10ab1c1

mvertens requested review from jmaerz and removed request for matsbn October 18, 2023 10:45

jmaerz mentioned this pull request Oct 18, 2023

Merging strategy for new release versions (CMIP6-compatible and INES interim release) #289

Closed

TomasTorsvik approved these changes Oct 18, 2023

View reviewed changes

mvertens added 2 commits October 19, 2023 13:54

added new tests for turning on all hamocc options

55a03c0

fixes for hamocc tests

90ca8ea

JorgSchwinger reviewed Oct 20, 2023

View reviewed changes

jmaerz mentioned this pull request Oct 20, 2023

Reorganize the model parameters into mo_param_bgc to enable usage of protected statement #292

Closed

updates requested in PR review

0a1082e

jmaerz reviewed Oct 23, 2023

View reviewed changes

jmaerz approved these changes Oct 23, 2023

View reviewed changes

added missing namelists

07b389f

TomasTorsvik self-assigned this Oct 24, 2023

TomasTorsvik added the iHAMOCC Issue mainly concerns the iHAMOCC code base label Oct 24, 2023

TomasTorsvik added this to the preparing for noresm2.1 milestone Oct 24, 2023

TomasTorsvik merged commit 1d71682 into NorESMhub:master Oct 24, 2023
12 checks passed

mvertens deleted the feature/refactor_ifdefs3 branch May 15, 2024 10:29

		# HAMOCC bromoform scheme currently only supported on the tnx1v4 grid
		# (no swa-climatology has been created for other grid configurations)"

migrate HAMOCC config settings to run time namelist settings #281

migrate HAMOCC config settings to run time namelist settings #281

Conversation

mvertens commented Oct 15, 2023 • edited Loading

JorgSchwinger commented Oct 16, 2023

mvertens commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvertens commented Oct 18, 2023

JorgSchwinger commented Oct 18, 2023

mvertens commented Oct 18, 2023

JorgSchwinger commented Oct 18, 2023

JorgSchwinger commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorgSchwinger commented Oct 19, 2023

mvertens commented Oct 19, 2023

JorgSchwinger commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaerz commented Oct 20, 2023

mvertens commented Oct 20, 2023

jmaerz commented Oct 20, 2023

mvertens commented Oct 20, 2023

jmaerz commented Oct 20, 2023 • edited Loading

jmaerz commented Oct 20, 2023

mvertens commented Oct 20, 2023

jmaerz commented Oct 20, 2023 • edited Loading

mvertens commented Oct 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaerz left a comment

Choose a reason for hiding this comment

TomasTorsvik commented Oct 24, 2023

mvertens commented Oct 24, 2023

jmaerz commented Oct 24, 2023

mvertens commented Oct 15, 2023 •

edited

Loading

jmaerz commented Oct 20, 2023 •

edited

Loading

jmaerz commented Oct 20, 2023 •

edited

Loading