fates-host parameter interface

[bandre-ucar]

Introduce fates-host parameter interface

Introduce an interface to pass parameter information from the host to fates. Fates registers a set of parameters that it needs read in, and indicates if they are fates only parameters, or need to be synced with values from the host. The host reads the parameters and returns them to fates.

This refactor attempted to be as minimally invasive as possible to the fates science code. All existing storage and conventions for fates parameters were left in place. The only exception was the consolidation of all pft dimensioned parameters into the EDpftvarcon type. Fates no longer uses variables from the host pftcon type.

This introduces dynamic allocation of pft level parameter in preparation for setting the number of pfts at run time, but still requires a hard coded number of pfts until the host code can be modified.

Note that the default clm and old clm-ed parameter files have diverged before this work began. To do this development in a bit for bit way, the clm-ed parameter file was updated to agree with the clm default parameter file. This is answer changing compared to the fates master branch, but code was refactored in a bit for bit way. No netcdf variables were added or removed in this PR.

Fixes: #155, #156

User interface changes?: Yes.

• Users will need to update custom parameter files. This introduces a new namelist variable, fates_paramfile. The fates parameters are always read from the netcdf file pointed to by fates_paramfile. All host parameters are always read from the netcdf file pointed to by paramfile. The host paramfile and fates paramfile may point to the same netcdf file.

• All fates parameters and dimensions are now name spaced by 'fates_'. The variable names have remained the same, but the dimension 'param' is now 'fates_scalar'. A new default parameter file with the required changes is available. See fates_params.c170308.nc in the input data repo.

Code review: self. Discussion with clm-cmt, code walk through with ckoven, rgknox, rosie

Testing:

andre:
    Test suite: ed - yellowstone gnu, intel, pgi
    Test baseline: n/a
    Test namelist changes: addition of fates_paramfile.
    Test answer changes: answer changing because of updated values in parameter file. See above.
    Test summary: all tests pass.

    Test suite: ed - hobart nag
    Test baseline: n/a
    Test namelist changes: addition of fates_paramfile.
    Test answer changes: answer changing because of updated values in parameter file. See above.
    Test summary: needs to be run a final time before accepting PR

    Test suite: clm_short - yellowstone gnu, intel, pgi
    Test baseline: clm4_5_12_r195
    Test namelist changes: addition of fates_paramfile.
    Test answer changes: answer changing because of updated values in parameter file. See above.
    Test summary: needs to be run a final time before accepting PR

andre: 2017-03-14

  Test suite: ed - yellowstone gnu, intel, pgi
                   hobart nag
  Test baseline: 0ea3fed
  Test namelist changes: addition of fates_paramfile
  Test answer changes: bit for bit
  Test status: all tests pass

  Test suite: clm_short - yellowstone gnu, intel, pgi
  Test baseline: clm4_5_12_r195
  Test namelist changes: none
  Test answer changes: bit for bit
  Test status: all tests pass

[rgknox]

awesome @bandre-ucar , looking forward to checking this out!

[ckoven]

great, @bandre-ucar. thanks-

[rgknox]

@bandre-ucar : I'm only starting to digest the changes, so bare with me. First off the design looks nice and clean, I like the generic register/recieve functions.

One question to start off:

1. we seem to be reliant on CLM's numrad right now, ie its called from clmvarcon as a global. One thing that I would advocate moving towards: could we allow the size of the dimension in the netcdf file to dictate what FATES believes to be the number of radiation bands? Note also that we pass a number of control-like parameters through the interface, see set_fates_ctrlparms(). One of the values that we currently pass through is the number of radiation bands that the HLM is aware of (which is numrad, but we rename it to be FATES global). We could then compare if the number of bands that are registerred with the host model matches the number of bands that we are detecting in the file, and fail if there is a mismatch. I hope I am not confusing this. And if you see detecting the dimension size as a stretch goal, I suppose for now we could just use the FATES equivalent "hlm_numSWb" as defined in PR Interface: endrun and cleaning of fates<->hlm globals #180, as a replacement for numrad for the time being.

[bandre-ucar]

@rgknox The numrad stuff is a mess. It is stored in the netcdf as a set of hard coded 1-d arrays, but then stored in fortran as 2d arrays. It really should be cleaned up, but I'd prefer that be done as part of the data cleanup and not this PR.

Maybe I'm not understanding the second part of your question but.... The 'use numrad' value can come from fates globals instead of clm as long as it's been initialized by the time it's needed. Most of the fates infrastructure get initialized long after pft parameters are read. Regardless of the source, the value gets used in allocating memory and dimension checks the same way. I don't think there are any additional checks that can be done until the hard coded 1d --> 2d arrays have been cleaned up. Or maybe I just don't understand your concern.

[rgknox]

Ah your right, I forgot that the hlm->fates transfer of those control parameters happens after the pft-parameter read-in. I think your echo'ing my point correctly, in that we need some FATES defined size of the dimension on those radiation arrays.
I suppose if we really want to break module dependency for the short term, we can simply define our own numrad as a constant parameter on the FATES side.
As for the allocation of the radiation parameter arrays; would it be possible to query the netcdf file for the size and use that to define the allocation? Is that what you were referring to as a clean-up task in a later PR or would that be different?

[bandre-ucar]

I think there is already an example of what you want with rootprof_beta(pft, variants)? It's a 2d variable where we don't specify the exact size of either dimension in the read code. It all comes from the file.

If the numrad variabes are combined into pft x numrad on the netcdf file instead of 2 pft x 1 arrays, then the dynamic allocation in fates parameters infrastructure will handle things correctly without specifying numrad at all. Then it's easy to add a dimension check after the control vars are available during fates initialization.

[rgknox]

ok right, I think I'm getting up to speed on what is happening. I can see in SetDimensionSizes() and its argument dimension_sizes() (which comes from the CLM side of the interface after querying the nc file) that array allocation of the parameter%data structures is dynamic and based on the array sizes in file.

So the reason we are using numrad is because the radiation variables in the file themselves are not structured as 2d arrays, but we want to use the model variables as such. If we did have them as 2d arrays in the file, then we could use the *PFT_nvariants structure, which calls RetreiveParameter2DAllocate() via generic procedure.

[rgknox]

do we save the size of the pft dimension anywhere to a explicitly named global variable?

I can't find if/where we store that number (aside from the arrays used_dimension_sizes in SetParameterDimensions() and the fates_params%parameter%dimension_sizes arrays).

If we don't, can you recommend or implement that @bandre-ucar ?

[rgknox]

ok nevermind, I see above that this PR is an incremental step towards that goal: "This introduces dynamic allocation of pft level parameter in preparation for setting the number of pfts at run time, but still requires a hard coded number of pfts until the host code can be modified."

[rgknox]

One next step that I think we had mentioned in our last SE meeting was to eventually get the shared parameters, the two q10's, in with the other parameters and break from the concept of shared parameters. Maybe this something I could implement after the bulk of the parameter interface is committed to help get more familiar with this new code.

[bandre-ucar]

@rgknox It depends on what you mean by "breaking with the concept of shared parameters". Fates absolutely needs to namespace it's parameters and stop reusing host variables for different purposes/values.

In this PR 'shared parameters' are values which are synchronized between the host and fates. e.g. physical constants or parameters where it would cause numerical inconsistency by having different values on both sides of the interface. Leveraging the new synchronized parameters infrastructure is going to be much less error prone than duplicating information in different places. It also provides a stable API that will require fewer backward incompatible changes than manually passing values between the host and fates in code.

I think what we discussed in the last SE meeting was that the q10 values don't meet that criteria, but other things do.

Should we rename 'shared' to 'synchronized'?

[rgknox]

@bandre-ucar, I think synchronized is maybe a better word, and I think your right there may exist parameters that should be synchronized between the HLM and FATES. If we can't come up with any now, that may be true in the future. I can't think of any good examples right now though, aside from physical constants. Maybe I am missing something though.
A question I am asking now is, should every parameter be governed by a model, be it FATES or the HLM, or other? Where in this case the value is specified by that module of origin via whichever parameter reading pathway it chooses, and other modules ask governing module for that parameter. Or, should different modules have some type of joint custody? I think I lean more towards the first, and think we should namespace the parameter to reflect its origin.
We may encounter this with @bchristo 's plant hydraulics scheme. There may be functions on the plant-hydraulics side of the code which calculates matric potential from water content and vice-versa, which need to have the same parameters that are used in the host model's calculation.

[bandre-ucar]

@rgknox Name spacing every parameter, including the host parameters, is a great idea. I'm not sure how to propose that change to alm and clm though.... We'd need to come up with a good common naming convention, lm_ or hlm_...? If we want to work out some details I'll start the discussion with clm.

I'll change EDSharedParams to FatesSynchronizedHostParams or something. Let me know if you have an idea for a better name.

[rgknox]

RE name-spacing, some of the dimensions and non-parameter globals being sent from HLM to FATES in ED/main/FatesInterfaceMod.F90:set_fates_ctrlparms() will have the "hlm_" string prepended to all names. Although this is added with PR #180.

[rgknox]

@bandre-ucar, I don't really have much more to comment on for my review. Its nice work and I think things look good. I will run tests on lawrencium. I have a few general final questions just to get a handle on the design and where its going:

1. I see that the call structure in FatesReadParams() is different for EDShareParamsInst, compared to SpitFire and Fates general params, ie Recieve and Register functions are part of the EDShareParamsInst class structure. Is this something you want to move towards with spitfire and the other fates parameters, or is just an arbitrarily different way of doing things?
2. Seeing how you split out spitfire parameters into their own sequence of registration and reception, is this design part of creating parameter groups that are only active when certain modules are turned on, or is this more just for keeping clean code and book-keeping?
3. I think I follow the pathway to reading in a "synced" parameter. I see two calls to ParametersFromNetCDF(), one for FATES dedicated file and one for the host's netcdf file. These synced/shared parameters are simply directed to be read through the host file.

[bandre-ucar]

1. I prefer going with the OO route and having the instance methods. But doing that were it didn't already exist is very invasive in the science code, so I didn't want to make that for the other parameters. I'd recommend moving the others in that direction as they are refactored. But it can be removed from the shared.

2. I created sub-calls in spit fire and pft parameters based on the dimensions of the variables that are being read. It keeps the code cleaner and less error prone because each function only has to deal with a single set of dimension names and shapes. That can easily be extended to functionality groups, but it wasn't the intention.

3. Yes. Putting the synched parameters in netcdf eliminates the need to explicitly and manually copy each synched variable through a the API. And it insulates the host from the internals of fates. I think's much more flexible and maintainable in the long run, but does require some initial agreement between the hosts and fates to move parameters into the file and have consistent naming.

[rgknox]

FYI, I added the parameter interface calls to my evolving interface call-graph:

https://www.lucidchart.com/documents/edit/cfe46676-b785-4443-8aba-288b1eca76c8/2

[ckoven]

@bandre-ucar these changes look good to me.

Also, I've started working from this PR to add the new parameters. A couple questions: when I add the new params, should I add them to a new netcdf file based off the file fates_params.c170209.nc that you've made? If so, and while there, should I cull the non-FATES variables from that file, or is that something you are planning to do?

[bandre-ucar]

@ckoven Yes, base any parameter changes off of fates_params.c170209.nc. You can cull non fates parameters. But talk to @rgknox first. He had some questions about simulation results, so we may need to coordinate changes.

[ckoven]

@bandre-ucar ok, thanks. yeah, i talked to @rgknox. is this PR as it currently stands meant to be bit-for-bit with respect to master if we were to put the current default FATES master parameter set into the new file format?

[bandre-ucar]

@ckoven None of the source code changes are expected to change answers. I did a lot of bit for bit testing to try to prevent that. But it's not going to be bit for bit with master. The clm_paramas_ed file had diverged from the clm_params file in some parameters. The files were split to have fates parameters come from the fates file and clm parameters come from the clm file, which changes answers compared to master.

[ckoven]

@bandre-ucar ok, great. so then the obvious/required test is to construct a new a new parameter file in the new format but with the same values as the one we are currently using to test master, make that our new default parameter file, and confirm bit-for-bitness. who is doing that?

[rgknox]

As per discussions offline with @bandre-ucar, we identified that fnitr in the new parameter file needs to be corrected to reflect he values in 160808, as well as BB_slope.

[rosiealice]

At some point, not necessarily as part of this, we should retire fnitr and replace it with a real 'vcmax25' parameter, since thatst what it really does... On Mar 7, 2017 4:12 PM, "Ryan Knox" <[email protected]> wrote: As per discussions offline with @bandre-ucar <https://github.com/bandre-ucar>, we identified that fnitr in the new parameter file needs to be corrected to reflect he values in 160808, as well as BB_slope. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMWsQy5s_K2ELkdXBARgejt-jiGIBcqUks5rjeRGgaJpZM4MJQVc> .

[rgknox]

good point @rosiealice

[bandre-ucar]

@rgknox I'm creating a new params file. In our off line discussion you mentioned the BB_slope and bb_slope parameters in the file. I thought you wanted me to switch to using the BB_slope value, but it is already being used. Can you clarify what change you wanted?

Also, there is a pesky artifact that is somehow still in the 160808 netcdf file, probably my fault, but ... there
as a BB_slope and a bb_slope in the parameter file.  Note that the master version of the code was using the
"BB_slope" value, not the lower case (crazy annoying, I know, its just silly).  So many things to do...

[rgknox]

sure thing,

I double checked the new file you made, and I see it contains both fates_BB_slope and fates_bb_slope. We only need one.

ncdump -h fates_params.c170209.nc | grep -i bb_slope
double fates_BB_slope(fates_pft) ;
double fates_bb_slope(fates_pft) ;

As far as I can tell, fates_BB_slope is the new namespace for the old "BB_slope" from netcdf 160808. That is the variable that should actually be used. So please drop fates_bb_slope (lowercase) from the new file, and double check that we are indeed using the upper-case named variable.

[bandre-ucar]

ok, fates_bb_slope lower case has been removed from fates_params_c170308.nc

[bandre-ucar]

@rgknox I updated the fnitr value in the parameter file. It didn't make the simulation bit for bit with what I think the parent should be. I need to look at more closely tomorrow. Can you also rerun your diagnostics on commit 47af51b an let me know if we are at least back in the ball park?

[rgknox]

will do!

[bandre-ucar]

New data point: I substituted the c170308 fates parameter file (revert fnitr changes) into 509fafa. A test with 509fafa is bit for bit with branch parent, d11608. So it looks like the merge to PR 174 in 3f3f16f to update with the trunk introduced an answer change.

edit - confirmed that the merge is the source of the answer change.

I think the problem was that the new file FatesParameterDerivedMod.F90 was still pointing to the host pftcon directly, and was using the host version of fnitr. Retesting.

[rgknox]

ok, thats helpful, I was in the middle of writing you an email mentioning that the 20 year diagnostics at 1x1 brazil were generating much different results than master. I will try and track down what happened on the merge as well. I'm guessing that it has something to do with the re-labelling of some of these dimension variables?

[rgknox]

It looks like you found the likely culprit, I will hold off on bug hunting until those tests are done.

[rgknox]

@bandre-ucar , I ran a 20 year cold-start regression test for the 1x1 brazil, updating the test to include the bug you caught in FatesParameterDerivedMod.F90 and your branch now generates indistinguishable results from master:

[bandre-ucar]

@rgknox Thanks! I ran full tests over night and they came back clean. I'll try to update the branch today after #184 is merged to master.

[ckoven]

@bandre-ucar , is there meant to be a distinction in usage between EDecophyscon and EDPftvarcon_inst? My reading of the code as it currently stands in this PR is that both are used pretty interchangeably to pass the PFT parameters into the code. Is that just a transient thing, or am I not perceiving an important difference between the two?

[bandre-ucar]

@ckoven It's a remnant of the original implementation. I personally would consolidate everything into EDPftvarcon_inst, but it was marked as being scheduled for removal. Either way, consolidation was vary invasive to the science code and wasn't appropriate for this PR.

[ckoven]

@bandre-ucar ok, thanks.