transfer most parameters into mo_param_bgc

[jmaerz]

Hi @JorgSchwinger and @TomasTorsvik , this is a first draft of transferring most model parameters as real,protected into mo_param_bgc - thus far, it is not cleaned up, untested, but compiles in single column mode. There are a few observations and thus questions, on how to best handle them:

1. I thus far declare and initialize the protected parameters at once - not sure, if this is the preferred way.
2. Thus far, I didn't initialize former pre-processor-dependent parameters at declaration - any suggestion, if we should do this and simply remove the if then...endif almost completely in mo_param_bgc (at least for the initialization procedures).
3. We need to initialize some parameters via calculations - I would suggest to transfer all of those after reading the namelist.
4. The namelist reading doesn't like un-initialized parameters which might be un-used afterward (e.g. the sinking speeds, due to former pre-processor flags) - hence we will have to initialize them via a dummy (e.g. -suggestion- real::dummy=-9999 and then use it for initializiation)
5. Constant wpoc, wopal, wcalc and wdust are and were used as locally changeable variable in ocprod (not the nicest practice..., preventing from setting them protected and in the namelist + making them local to OMP) - I would suggest to declare (and initialize) them now as wpocfix, etc. and use the former as local variables in ocprod.
6. There are a few parameters that are similarly strangely used (and some that appear to be changed in loops, e.g. growth_co2, bifr13_perm,atm_cfc11_nh... Partially, I am not sure, whether they are somewhat protected when OMP is used....
7. I will update the write statements for the use_xxxx so that the logs will also enable to check, which config setting were used.
8. Since the buildnml via xml-change includes already a number of model parameters: is there a need or wish, to include them in the namelist read?

closes #292

I would appreciate comments :-)

[JorgSchwinger]

Is there a reason for not including atm_CO2 in this module?

[jmaerz]

To me, no - but since it was thus far initialized in bgcnml , I didn't move it at first place also in the wish to not break potentially older scripts that make use of this nml. But I can move it, no problem.

[JorgSchwinger]

I would tend to have tf* defined as parameters - these come from a specific parameterization, so I wouldn't think there is a need to have them as tunable parameters? (Anyway, this is obsolete with the new N-cycle, I guess)

[jmaerz]

Well, they are still used - also with the N-cycle. Since there might be the wish to run bluefix only in the euphotic zone with the new hybrid coordinates, there might be the wish to tune them (while I suspect, we won't). Hence, I would be a bit hesitant to set these parameters a parameter.

[JorgSchwinger]

This can definitely be a parameter, I think - this is known with relatively little uncertainty, so not used for tuning.

[jmaerz]

Done for c14_t_half.

[JorgSchwinger]

I agree it makes sense to rename them wpocfix,... or similar

[JorgSchwinger]

I guess this whole routine will be removed eventually?

[jmaerz]

Yes, done.

[JorgSchwinger]

The bromoform stuff could be initialized here, couldn't it?

[jmaerz]

Yes, done.

[JorgSchwinger]

Why is atten_C not initialized here? I see ctochl would need to be defined as a parameter then, but that would be fine in my oppinion.

[jmaerz]

Agreed, done.

[JorgSchwinger]

1. I thus far declare and initialize the protected parameters at once - not sure, if this is the preferred way.

Agree, I also prefer this

2. Thus far, I didn't initialize former pre-processor-dependent parameters at declaration - any suggestion, if we should do this and simply remove the `if then`...`endif` almost completely in `mo_param_bgc` (at least for the initialization procedures).

I would support to remove most (or even all) if then and keep it simple. Doesn't hurt if a scalar variable is declared but not used because an option isn't switched on

3. We need to initialize some parameters via calculations - I would suggest to transfer all of those **after** reading the namelist.

If the calculation only involves parameter variables, I would suggest to keep the initialization in the module header, otherwise I agree.

4. The namelist reading doesn't like un-initialized parameters which might be un-used afterward (e.g. the sinking speeds, due to former pre-processor flags) - hence we will have to initialize them via a dummy (e.g. -suggestion- real::`dummy=-9999` and then use it for initializiation)

I don't understand this, un-initialized variables can be read from a namelist, can't they?

5. Constant `wpoc`, `wopal`, `wcalc` and `wdust` are and were used as locally changeable variable in `ocprod` (not the nicest practice..., preventing from setting them `protected` **and** in the namelist + making them local to OMP) - I would suggest to declare (and initialize) them now as `wpocfix`, etc. and use the former as local variables in `ocprod`.

Agree with renaming them.

6. There are a few parameters that are similarly strangely used (and some that appear to be changed in loops, e.g. `growth_co2`, `bifr13_perm`,`atm_cfc11_nh`... Partially, I am not sure, whether they are somewhat protected when OMP is used....

I think these are more local variables than parameters, we should check whether it is necessary to initialize them at al?

7. I will update the write statements for the `use_xxxx` so that the logs will also enable to check, which config setting were used.

Good idea, thank you

8. Since the buildnml via xml-change includes already a number of model parameters: is there a need or wish, to include them in the namelist read?

Good point to discuss, but maybe keep this out of this PR?

[jmaerz]

Hi @JorgSchwinger , thanks for the input, here some more explanation:

3. We need to initialize some parameters via calculations - I would suggest to transfer all of those **after** reading the namelist.

If the calculation only involves parameter variables, I would suggest to keep the initialization in the module header, otherwise I agree.

I fear that this isn't possible per Fortran 2008 standard: executable statements are not allowed in module headers - only if the variable used in the statement is previously declared as parameter (and not as protected), the declaration and initialization seem to be allowed (see also: https://stackoverflow.com/questions/45086892/unexpected-assignment-statement-in-modules). So, to my understanding, we will rely on initializing them in a subroutine, if we want to keep it flexible via namelist read.

[TomasTorsvik]

Is it intended that all variables in this public block will be declared either parameter or protected eventually?

[jmaerz]

Yes, that's what I/we am/are aiming at. Currently there are a few parameters, where this wasn't possible since they are used as changeable parameters in other subroutines, though (which is upon change).

[TomasTorsvik]

@jmaerz
I agree with your suggestions for changes 1-7, and for leaving 8 for another day.

[JorgSchwinger]

Re point 3, I just thought there are some variables that could/should be decleared as parameter and then some calculations could also be kept in header

[jmaerz]

Re point 3, I just thought there are some variables that could/should be decleared as parameter and then some calculations could also be kept in header

Well, depends, how flexible one wants to go - if we are ending up with having more and more (variable) parameters in the namelist, then this won't work and would thus be a reason to keep the structure as is (see also the updated code).

Thus far, I simplified the dust sinking a bit (since code was doubled - only the time point of *dtb was different), but I refrained from integrating the initialization of the aggregation scheme by Iris in the general routines. I'll work on the parameter replacement in ocprod now.

[jmaerz]

I just realized that there were a few more comments from your side @JorgSchwinger - I'll work them in.

[jmaerz]

Hi @TomasTorsvik and @JorgSchwinger ,
since we talked about it earlier this year, I now also moved the initialization of the atmosphere fields into mo_ini_fields (former beleg_vars).

Currently, atm_co2 is set via the bgc_namelist, while the BGCPARAMS is set by default as empty by the new python-based buildnml. Hence, I kept the atm_co2 outside the mo_param_bgc for now - and we should discuss, how we handle the parameter namelist (BGCPARAMS) further - I am not sure, if there is a possibility to only write a few of the values listed in the xml-file with the nml-definitions.

Further, since there is a bug in the sinking scheme anyway (dust sinking), I would suggest to handle the wpoc, etc. later in the beyond-CMIP6 branch (also because I am a bit in unease, whether we break the response of the OMP). If really needed, I can spend time looking into it now, but also for merging, it will be easier, carrying out these smaller fixes later. Similarly for bifr13, bifr14, etc. since they all affect handling with OMP.

I will try to perform the regression testing tmw.

[jmaerz]

Hi @TomasTorsvik and @JorgSchwinger , after one fix of the code, the results of the runs carried out via the regression testing are bit identical (was running just the SMS_D_Ld1.T62_tn14.NOINYOC.betzy_intel test for now). Eventually, I ended up comparing the output files manually - somehow, I have issues to make the comparison after running the codes automatically - seems to be some path issues in my script for the testing. I used:

#!/usr/bin/bash
#
# Simple testscript for testing regression of code
# see also: https://github.com/NorESMhub/BLOM/wiki/BLOM-iHAMOCC-regression-testing-with-NorESM
#

CHKFLD="chk-restructure-params"
BASENAME="NorESM"
CHKNAME="NorESM-to-chk"

TESTSUITE="SMS_D_Ld1.T62_tn14.NOINYOC.betzy_intel"

PROJECT="nn2980k"
BASELINEROOT="/cluster/work/users/jma105/noresm/"
BASELINENAME="baseline_${TESTSUITE}" 
ROOTFLD="/cluster/projects/${PROJECT}/${USER}/${CHKFLD}"
BASEROOT="${ROOTFLD}/${BASENAME}"
CHKROOT="${ROOTFLD}/${CHKNAME}"


# Creating the new baseline:
$BASEROOT/cime/scripts/create_test ${TESTSUITE}                     \
                                   --xml-machine betzy              \
                                   --xml-compiler intel             \
                                   --generate ${BASELINENAME}       \
                                   --baseline-root ${BASELINEROOT}  \
                                   --project ${PROJECT}

# Creating test for branch to be checked:
$CHKROOT/cime/scripts/create_test  ${TESTSUITE}                     \
                                   --xml-machine betzy              \
                                   --xml-compiler intel             \
                                   --compare ${BASELINENAME}        \
                                   --baseline-root ${BASELINEROOT}  \
                                   --project ${PROJECT}

and the automatic testing tries then to compare to baseline_SMS_D_Ld1.T62_tn14.NOINYOC.betzy_intel - while this folder doesn't contain any output files for the base run (the two runs are just carried out as usual, but with a different folder layout and not pushing anything to archive afterward). I guess, I am still missing a point how to set up the testing correctly. Anyway, the runs look good to me.

One thing, though: shall we also transfer the sediment-related rates and parameters into mo_param_bgc?

[TomasTorsvik]

@jmaerz
Thanks for testing. Maybe it's a good idea to also transfer the sediment parameters, but I would prefer to do it in a new PR, and merge this as it is.

[jmaerz]

@TomasTorsvik , would you suggest to do the sediment parameter transfer for the milestone? - or thereafter?

[TomasTorsvik]

@jmaerz , it would be nice to include the transfer of sediment parameters for the milestone if possible. It seems the atmospheric component is somewhat delayed for noresm2.1, and we would rather take an extra month or two to get it in good shape rather than to rush through with a release. It was a bit open ended during the noresm-core meeting today, but that was the impression I had from the meting. So, probably, we have some more time for the BLOM milestone as well.

[jmaerz]

Hi @TomasTorsvik , ok, good to know - it shouldn't take too long - the general layout is there now.

[gold2718]

@jmaerz,
A pattern I use a lot is the equivalent of:

# Creating the new baseline:
$BASEROOT/cime/scripts/create_test ${TESTSUITE}                     \
                                   --xml-machine betzy              \
                                   --xml-compiler intel             \
                                   --compare ${PREVIOUS_BASELINENAME}       \
                                   --generate ${BASELINENAME}       \
                                   --baseline-root ${BASELINEROOT}  \
                                   --project ${PROJECT}

where I made up a new variable name, PREVIOUS_BASELINENAME, to show that I am comparing to the old baseline while also generating a new baseline.

[jmaerz]

Dear @gold2718 , many thanks for this, but if you need a new baseline (not having an old one to compare to), what's wrong with the script above? The cases were created (different folder structure than regular), but the comparison failed since cime seemed to expect the folder structure differently (if I remember correctly, 2 runs where created - with the nc files and one testbase folder which cime tried to compare with, while not finding the nc files there). Do I need to manually re-organize the base to which to compare to (as in: moving the run for the baseline manually) or would you expect that cime is doing this automatically?

[gold2718]

Maybe I am misunderstanding (easy since I am late to the discussion) but isn't the compare baseline from an older tag (or hash)?
If that is the case, then to generate a previous baseline and then use it for a compare, you would need something like this:

#!/usr/bin/bash
#
# Simple testscript for testing regression of code
# see also: https://github.com/NorESMhub/BLOM/wiki/BLOM-iHAMOCC-regression-testing-with-NorESM
#

CHKFLD="chk-restructure-params"
BASENAME="NorESM"
CHKNAME="NorESM-to-chk"

TESTSUITE="SMS_D_Ld1.T62_tn14.NOINYOC.betzy_intel"

PROJECT="nn2980k"
BASELINEROOT="/cluster/work/users/jma105/noresm/"
BASELINENAME="baseline_${TESTSUITE}" 
PREVBASELINENAME="prev_baseline_${TESTSUITE}" 
ROOTFLD="/cluster/projects/${PROJECT}/${USER}/${CHKFLD}"
BASEROOT="${ROOTFLD}/${BASENAME}"
CHKROOT="${ROOTFLD}/${CHKNAME}"

OLDTAG="??"
NEWTAG="??"

# Make sure the previous (to be compared to tag (hash) is checked out
git checkout ${OLDTAG}
./manage_externals/checkout_externals

# Creating a baseline for comparison:
$BASEROOT/cime/scripts/create_test ${TESTSUITE}                     \
                                   --xml-machine betzy              \
                                   --xml-compiler intel             \
                                   --generate ${PREVBASELINENAME}   \
                                   --baseline-root ${BASELINEROOT}  \
                                   --project ${PROJECT}

# Make sure the new (to be tested) tag (hash) is checked out
git checkout ${NEWTAG}
./manage_externals/checkout_externals

# Creating test for branch to be checked:
$CHKROOT/cime/scripts/create_test  ${TESTSUITE}                     \
                                   --xml-machine betzy              \
                                   --xml-compiler intel             \
                                   --generate ${BASELINENAME}       \
                                   --compare ${PREVBASELINENAME}    \
                                   --baseline-root ${BASELINEROOT}  \
                                   --project ${PROJECT}

If I am still off the mark, please let me know what I am missing.

[jmaerz]

Dear @gold2718 , many thanks for looking into it. I am a bit confused about the second command: why would you want to generate another baseline - or is that needed? -note that I assume the runs of $PREVBASELINENAME and ${BASELINENAME} in your script should be bit identical. All the checkouts I did manually before in the two different NorESM folders (BASENAME and CHKNAME). I guess, I should give it another try and see, if it fails again and then one can look into it together - then it becomes easier to explain (and to analyze).
My suspicion is/was that the initial baseline generation failed to copy the generated run/*.nc output into the folder given by the --generate command. I ran my script twice - one time, indeed, files were not identical - which I discovered manually. After fixing the bug and deleting all former output, the second time the generated *nc files were identical, while the testing was still failing again for the reason that the baseline run somehow wasn't found (at least that's how I read the provided information). The nc files for the baseline were somehow expected under the folder baseline_... while I found the *nc files under just the run name (the SMS_D_... testsuite name) in the baseline root folder (not baseline_...).
Anyway, thanks again for looking into this. At least, from what I gathered from your comment is that you would expect the commands to function as I also would expect them to function. Hence, something in either the used cime version is fishy or I overlook some small detail in the shell script. I will dig into it the next days.

[gold2718]

I think I am starting to understand (I'm feeling slow today). It sounds like you are not interested in storing a baseline.
We usually store the baselines for a tag so that we can quickly see if a new development is causing any trouble for existing supported configurations. That is why I had the --generate flag in the second step. This is totally up to you and your team.

However, the real problem here (which I keep failing to grasp) seems to be that there is some baseline format change between the tag you are testing and the tag you are comparing to. Is this accurate?

Can you point me to the baseline directory and the test directory (if they are available on a Sigma2 machine)? I would like to look around to better understand what happened and how we might address it.

[jmaerz]

Dear @gold2718 , sorry for getting back to this that late. I fear, I didn't took enough time to dig deeper into the test results and at least, most things work as expected. However, one thing remains: in the cprnc.out file, just the information /bin/sh: UNSET: command not found is printed (which may then populate into the issue tagging the comparison as failed). See e.g /cluster/work/users/jma105/noresm/SMS_D_Ld1.T62_tn14.NOINYOC.betzy_intel.C.20231107_152331_tn0ivn.blom.hd.0001-01-01.nc.cprnc.out - I am not sure, if this issue is caused by cime at all...

[mvertens]

@jmaerz - thanks for the pointer to your cprnc.out. That is very odd. @gold2718 and I will look into it.

[mvertens]

In your $CASEROOT - I did the following:

> ./xmlquery -p CPRNC
And got the following:
CCSM_CPRNC: UNSET
The problem is that CCSM_CPRNC: UNSET

So the problem is that you are not pointing to a cprnc executable. That was never created.
I think this means that no regression testing with CIME was carried out with the release.
Could you try the following in your $CASEROOT:

./xmlchange CCSM_CPRNC=/cluster/shared/noresm/tools/cprnc/cprnc

We need to update CIME for the release so that it contains a pointer to this file.
In the meantime - any test you do with this code will need for you to do the above xmlchange command in your $CASEROOT.
Does that make sense?

[jmaerz]

Dear @mvertens , thanks for the info. From ./create_test -h I see that there is likely no option to directly change this setting via a --xmlchange - so the only option perform these tests is set --no-run, then do the xml-changes and manually start the tests. Or do I overlook something?

[mvertens]

@jmaerz - I think I have a simple solution.
In your file - $SRCROOT/cime/config/cesm/machines/config_machines.xml
Replace the CCSM_CPRNC block under the betzy machine as follows:

 <machine MACH="betzy">
       ......
    <CCSM_CPRNC>UNSET</CCSM_CPRNC>

with

<CCSM_CPRNC>/cluster/shared/noresm/tools/cprnc/cprnc</CCSM_CPRNC>

Then try to create a new case and see if this works. We will make a new release tag with this change.

[jmaerz]

Dear @mvertens, thanks, that sounds convenient - I'll try this out.