NorESMhub / BLOM

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

Namelist `/bgcparams/` writing via `buildnml` #327

Closed jmaerz closed 5 months ago

jmaerz commented 5 months ago

Dear @mvertens , @JorgSchwinger , @TomasTorsvik , with bringing in the code changes done last autumn to my nitrogen cycle branch, I am facing the issue that model parameters set via the /bgcparams/ namelist cannot be changed via the python-based buildnml file (it should be the same issue for current master). It requires to modify a few lines in buildnml to make that happen (otherwise, it just fails). Once done, any parameter found in namelist_definition_blom.xml belonging to /bgcparams/ will be written to /bgcparams/. In my opinion, this clutters information on which parameters have been tuned via user_nl_blom. We currently hard-coded default parameters in mo_param_bgc.F90 by purpose and I wonder, if we should only write parameters to /bgcparams/ that were found in user_nl_blom to keep the entries in /bgcparams/ as informative as possible and to not having to maintain two places with correct/identical parameter values (i.e. both, mo_param_bgc.F90 and namelist_definition_blom.xml). It should be rather simple to strip off other parameters via the ocn_in_paramgen.py. Any opinion about it? Or is there a push to set all parameters via the xml-file (I could see some benefits, but being rather hesitant in this)?

mvertens commented 5 months ago

@jmaerz - the type of output you would like that ONLY writes the namelist values that are changed actually would require quite a bit of rewrite of the namelist generation code - so that is not feasible. The namelist generation capability across CESM and NORESM requires that any parameter found in namelist_definition_blom.xml belonging to /bgcparams/ will be written to /bgcparams/ IF /bgcparams/ is written. However, it seems that if you just look at the values in user_nl_blom - you should be able to easily tell which values have been changed. A separate small ascii file could also be written to $RUNDIR documenting this for blom if that would be useful. In addition, the ability to actually write out the namelist for bgcparams should at this point always be turned on I think in buildnml. I thought that the empty bgcparams was just a place holder for what you are doing now.

mvertens commented 5 months ago

@jmaerz - thinking about this some more it would be not be a good idea to ever just write out the changed namelists. The reason is that you want the entire namelist to be written out based on the default values that the namelist generation tool gives you. Ideally you don't want hard-coded namelists in your code mixed with namelists defaults where you have to look at 2 places to determine what the actual value is. So if you don't have an empty namelist - you should always write out the full namelist and change the values you want changed in user_nl_blom. This will ensure that you are getting the actual namelist values you expect and not some unexpected behavior that was not detected because you only had a subset of namelist values set. Does that make sense?

mvertens commented 5 months ago

@jmaerz - to clarify - to write out JUST the changed namelist rather than the full namelist is not a good idea.

gold2718 commented 5 months ago

@jmaerz, CESM-CAM has wrestled with this issue for literally decades and has the following best practice.

I hope this info is useful to the BLOM group.

In my opinion, this clutters information on which parameters have been tuned via user_nl_blom

I am a little confused by this statement. user_nl_blom is the authoritative source for all deviations from the "preset" values. It is only as cluttered as the user makes it. What am I missing?

JorgSchwinger commented 5 months ago

I think the issue of the default values boils down to the question whether we still want to maintain a stand-alone BLOM/iHAMOCC, which is able to run outside the CESM/NorESM framework. Currently this is possible, and therefore we have all iHAMOCC namelist default values hardcoded. With the the new namelist_definition_blom.xml, we have a new, second, levels of default values, which isn't optimal.

Regarding writing out the values: I think having the ocn_in file plus the log-files (where the actually used parameters are written out) should be good enough, shouldn't it?

jmaerz commented 5 months ago

Dear @mvertens , I understand and agree to most of your points if (and only if) the xml-file holds the default(!) values. Thus far, the /bgcparams/ has been seen and Fortran-written in a way that it allows to tune parameters, while isn't needed to load default parameters. The only benefit I see to have the parameters written in the xml file is the situation, when the default parameters NEED to be different for different setups (e.g. through different resolution, different coordinate system like the hybrid-versus regular coordinates, etc. - currently, there is no experience in that to my knowledge and there was ONE set of default parameters hard-coded, which provides a better overview, is safe wrt unwanted user changes and is thus rather conservative). Hence, the present understanding was to enable tuning by only(!) the expert user via /bgcparams/, while once a default parameter set is found, it enters the Fortran code that is less prone to python errors and user changes. Given the current code situation, we have to maintain two places with default parameters, if we switch to include all /bgcparams/ values. Hence bringing this up. @gold2718 : I didn't mean the user_nl_blom being cluttered - in the ocn_in file the namelist for -thus far - only as tuning parameters understood namelist /bgcparams/ "get cluttered" with (so far anyway Fortran hard-coded) default parameters. It is something to decide among the HAMOCC group. Note that this is ONLY about the /bgcparams/ namelist (not the other namelists used by iHAMOCC).

mvertens commented 5 months ago

@jmaerz - the xml defaults file should always hold the default values - even in experimentation. I do not agree that the only benefit is when the default parameters are for different setups. The key point is that the default values are set in one and only one place - and all namelist settings come as run time values and not some that are set in the code. Hard-coded parameters have caused numerous problems in my experience with all components in CESM and @gold2718 can in particular comment on his experience with CAM. Having a workflow that is consistent for both experts and novices will lead to the most robust code base.

jmaerz commented 5 months ago

Right, that's why they were thus far consistently set in mo_param_bgc and we have to decide how to best proceed.

mvertens commented 5 months ago

@JorgSchwinger - sorry I did not see your message before I replied to @jmaerz. You raise an excellent point - and one that I did not think of. So in that case it does make sense to have the hard-coded values. But maybe moving forwards we can have an #ifdef section that separates the NorESM versus the stand-alone blom default settings. In the noresm case - they all come in from the namelist_defaults and have uninitialized values and in the stand-alone they are hard-coded. That way - the namelists are set in one and only one place when you run the model. Thoughts?

gold2718 commented 5 months ago

@JorgSchwinger, One module that I have seen address this issue uses the following method (rough sketch with one nameliet variable below):

real, parameter :: invalid_real = -HUGE(1.0) ! The runtime parameter used in the code
real :: runtime_parameter = invalid_real ! The runtime parameter used in the code

real :: runtime_parameter_defval = 273.16

subroutine read_namelist(namelist file)
    . . . 
   namelist /my_namelist/ runtime_parameter

   if (standalone) then
      runtime_parameter = runtime_parameter_defval
   end if

   ! Open file, find group
   read (funit, my_namelist)

   if (.not. standalone) then
      if (runtime_parameter == invalid_real) then
         logerror('runtime_parameter not set by my_namelist')
      else if (runtime_parameter /= runtime_parameter_defval) then
         write(logunit, *) 'runtime_parameter' changed from ', runtime_parameter_defval, ' to ', runtime_parameter
      end if
   end if

This has all the 'standalone' default values in one place and uses those same values to flag any value changed from the standard default during a model run.

jmaerz commented 5 months ago

Dear @mvertens , @JorgSchwinger , I couldn't hold back and was looking a bit into the buildnml and ocn_in_param_gen files to see, how one can tackle the issue there. I found a solution that is of minimal invasion into the code, see #328. If anyone wants to improve the solution, I am happy to learn. This is just a suggestion thus far and doesn't mean any decision.

jmaerz commented 5 months ago

Ok, after having a brief discussion with @mvertens , I will push the interim solution. Whenever there is time and a more elaborated solution decided based on further discussion, we can easily modify, rollback and/or expand on the interim solution. For now, it suits the needs to enable tuning possibilities through user_nl_blom.