direct usage of parameter datastructure - allometry proof of concept

[rgknox]

Description:

With the introduction of the parameter data structure, we can access parameters directly from that structure and bypass the middle-man, ie the allocated data primitive arrays that we fill from the parameter data structure.

Following initial review and discussion, the relevant data primitives will be removed.

Collaborators:

Expectation of Answer Changes:

This should not change any answers, B4B

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

Contributor

• The in-code documentation has been updated with descriptive comments
• The documentation has been assessed to determine if updates are necessary

Integrator

• FATES PASS/FAIL regression tests were run
• Evaluation of test results for answer changes was performed and results provided
• FATES-CLM6 Code Freeze: satellite phenology regression tests are b4b

If satellite phenology regressions are not b4b, please hold merge and notify the FATES development team.

Documentation

• Technical Note update:
• User's Guide update:

Test Results:

TBD

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

[rgknox]

I have a prototype of an alternative approach, see this commit:
rgknox@54da968

In this approach, the idea more similar to our previous approach. Every parameter is associated with a uniquely named primitive data structure, with a few key differences.

1. These are pointers, but not allocated pointers, because each points to the datastructure generated in the JSON parser which stays allocated. (I think it is better to keep the original structure intact, rather than transfer and deallocate, because it contains all the metadata, dimension names, etc, which is useful to keep)

2. These pointers have a protected status (this will prevent the pointer's association from changing outside the module), and I think i need to label them "protected" for each parameter I define, and not at the derived type level...

3. All fates parameter pointers are defined in one data structure in main/FatesParametersInterfaceMod.F90 (this would be completed in following steps, ie consolidate all parameters in fire,edpftvar,edparams,rad, parteh,etc)

4. the new data structure can either be a stand-alone, or an extension of the data structure defined in main/JSON (two options provided)

[adrifoster]

Thanks for doing this Ryan. I really appreciate the effort to try to clean up our handling of parameters in FATES, as it has always felt a little haphazard to me.

I guess I was hoping to discuss the problems your PR is hoping to solve. It seems like the main one is your goal of removing the "middle man" problem. However, I guess I see it differently. From my perspective, it is a good idea to have two separate classes (or types of classes) to handle the parameters:

(1) read/parse the JSON file, handle string lookups, deal with raw data, etc. (i.e. your params_type)
(2) holding the parameters in an easily accessible, strongly typed and obvious way (i.e. something like params%vcmax25top)

The readability and debugging feels easier to me if the parameters are held in the second class. Right now if we use the params_type method scientists/developers and every method that uses it has to deal with how to access the parameters (because we need the entire pstruct%parameters(pid_allom_agb1)%r_data_1d(ipft)) rather than just what (params%agb1(ipft)). I feel the second way makes the cognitive load lower and thus coding faster, and fewer typos probably. The second method is fairly self-describing, whereas the first seems like it requires a learning curve.

Additionally, if our I/O method ever changes, we do not have to re-factor the entire model, just where it's translated to the "holder" class.

What I would really like to see with the parameters class(es) is either have one class that holds all the parameters in named variables, or several (one for each "category"), i.e. leaf_params, fire_params, etc., though I feel like this needs to be fairly well constrained.

The idea here would be that we initialize the pstruct class, use it to read in parameters and fill the appropriate "holder" class(es), and then deallocate pstruct. This shouldn't take too much runtime because they are just single numbers.

But I'd like to talk more about this since I realize I am suggesting something really different than what you wrote.

pinging other folks: @ekluzek, @mvertens

[rgknox]

Thanks for your thoughts @adrifoster , yeah, I'm curious what others think on this too.

I also like the idea of moving back to one data structure to hold all parameter constants. So if we do go with the plan of transferring to the holder data structure, and removing the json data structure, lets stick with one structure, instanced in FatesParametersInterfaceMod, which has almost no dependencies.

Something we could do to mitigate some of the issues described thus far:

If we went with maintaining just the "pstruct" data structure, the index that identifies a parameter could have the same exact syntax as the parameter name in the file: for instance the parameter 'fates_leaf_vcmax25top', could use integer index fates_leaf_vcmax25top. This would actually reduce the number of names used, as right now we call this %vcmax25top. This would reduce ambiguity, we only need remember the name as it appears in the file, should be easily searchable in the code.

That being said, the part that I also don't like about exclusively using the pstruct% json data structure to access the parameters in the code, is that we will dealing with these different array types in our code: %r_data_1d, sometimes %i_data_1d, etc ... It doesn't come across as very clean, and people might not understand what is going on. Maybe I'm coming around on this idea of using a holder structure and deallocating the pstruct json structure.

[glemieux]

Adding my thoughts:

If we went with maintaining just the "pstruct" data structure, the index that identifies a parameter could have the same exact syntax as the parameter name in the file: for instance the parameter 'fates_leaf_vcmax25top', could use integer index fates_leaf_vcmax25top. This would actually reduce the number of names used, as right now we call this %vcmax25top. This would reduce ambiguity, we only need remember the name as it appears in the file, should be easily searchable in the code.

I really like the idea of aligning the parameter names so they are closer if not exactly the same.

That being said, the part that I also don't like about exclusively using the pstruct% json data structure to access the parameters in the code, is that we will dealing with these different array types in our code: %r_data_1d, sometimes %i_data_1d, etc ... It doesn't come across as very clean, and people might not understand what is going on.

I was thinking this as well. Initially I was going to suggest some sort of function call with the parameter ID as input argument to return the data, but I think that might be overkill in this particular instance.

I want to 👍 your suggestion to use protected for the held parameter values as well @rgknox. Making sure we avoid accidentally overriding the read parameters is critical. My assumption is that we don't do any "post-processing" of parameter data after read in, but if we did, it probably should be held privately in the module and type methods for the JSON IO.

In thinking about this, I really like maintaining a "separation of concerns" here with the design as suggested by @adrifoster, i.e. having a derived type for IO and a separate derived type for accessing the data. So I think @rgknox suggestion of creating a new type with a pointer to the params_type would work well to accomplish the separation. I don't think extending the params_type, which was one of the suggestions in rgknox@54da968, makes sense since I would ideally like to avoid us having access to the JSON methods when we're trying to use the parameter data. I don't feel like I know enough to state definitively if copying the data into the new derived type and then deallocating the IO type is more performant or "safer" than keeping the IO type around and creating a new derived type to point at the IO type data.

I do strongly agree that we should keep the parameter metadata, specifically the units. I would like us eventually to develop (or make use of) a module that handles unit conversions in a more automated and safe manner. Keeping the units stored with the parameters would help move us in that direction.