Closed rgknox closed 8 months ago
Most of my comments are questions asking for clarification. The few requested changes are fairly minor, so I leave those at your discretion. Note I only skimmed the unit testing code (which I loved to see though!).
I have no idea how this got added to this PR. It's a match to the approval I wrote for 2 stream. Must be a weird github glitch :shrug: ? I did reference is in a review comment, and some of the code from here appears in 2 stream, so maybe that's why?
Preliminary tests are passing on derecho. I compared one test with base (SMS_1x1_brazil) and that showed differences within expected tolerances. This PR has no math changes, but it does have order of operations changes and plenty of shuffling around, so I do not expect B4B. The diffs that i saw were typically smaller than abolute and relative e-10. In light of this, I'm removing the draft status and looking for reviewers.
Also: I currently only have things split out to level "2". ie, I don't differentiate multiplexed output in that fourth (last) dimension, from output that is not multiplexed in the fourth dimension. My plan is make sure all test are passing before I do that split.
Concern raised by @ekluzek that "density" can be confusing. E.g., does this refer to single vs. double precision?
@samsrabin I updated language to use "dimension" instead of density
all tests (fates and aux_clm) are ok on derecho and izumi. Small diffs attributed to order of operations, other diffs related to targetted fixes in a small and known set of variables. Some diffs associated with two-stream tests, which is a separate issue. /glade/derecho/scratch/rgknox/tests_0312-112335de /scratch/cluster/rgknox/tests_0312-211553iz
Description:
1) This allows FATES output to be controlled by setting density levels in the ELM/CLM namelist. Right now, the namelist setting is: "fates_hist_dense_level". The input is a list of two comma delimited integers. The first digit controls fates output at model timesteps (hi-frequency) and the second digit controls output at the dynamics (daily) timestep.
2) A cross-referencing routine has been added. This will loop through the list of variables that are in the master list from the HLM, and see if that variable has been allocated on the FATES side. It is now possible there could be a discrepancy. There will be a graceful and explanatory fail if the variable is not allocated.
3) This test will definitely require changes to the testing. For our "allvars" type tests, we should allocate all and use the highest output density levels (2,2). Perhaps we should have more allvars, and then run more tests at decreased level (1,1). At least one test should be 0,0.
4) Timing tests forthcoming
synchronized with: https://github.com/ESCOMP/CTSM/pull/2339
user documentation updates here: https://github.com/NGEET/fates-users-guide/pull/59
Here is a description of the namelist control from clm_varctl.F90:
CTSM-side changes: https://github.com/rgknox/ctsm/tree/fates-hist-density
Notes: This set of changes is built on-top of the two-stream pr. This set of changes will undboubtedly have conflicts with the LU PRs, but @ckoven and I have identified the differences and they won't be hard to incorporate.
Collaborators:
@ckoven
Expectation of Answer Changes:
Ideally, this should be a b4b set of changes, although it is possible with all the loop chopping being done, some things might have very small roundoff diffs.
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:
NOTE: THIS UPDATE SHOULD BE ADDED TO THE USERS GUIDE, AND HAVE APPROPRIATE PR WHEN INTEGRATING THIS ONE
Contributor
Integrator
Documentation
Test Results:
CTSM (or) E3SM (specify which) test hash-tag:
CTSM (or) E3SM (specify which) baseline hash-tag:
FATES baseline hash-tag:
Test Output: