Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

user controlled history density #1119

Merged
merged 28 commits into from
Mar 13, 2024
Merged

user controlled history density #1119

merged 28 commits into from
Mar 13, 2024

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Nov 13, 2023

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: ESCOMP/CTSM#2339

user documentation updates here: NGEET/fates-users-guide#59

Here is a description of the namelist control from clm_varctl.F90:

  ! FATES history density level
  ! fates can produce history at either the daily timescale (dynamics)
  ! and the model step timescale. It can also generate output on the extra dimension
  ! Performing this output can be expensive, so we allow different history density
  ! levels.
  ! The first index is output at the model timescale
  ! The second index is output at the dynamics (daily) timescale      
  ! 0 - no output
  ! 1 - include only column level means
  ! 2 - include only output with only 1 additional dimension
  ! 3 - include all multiplexed dimensions

  integer, dimension(2), public   :: fates_hist_dense_level = (/1,1/)

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

  • 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

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:

@rgknox rgknox added the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Nov 20, 2023
glemieux

This comment was marked as outdated.

@glemieux
Copy link
Contributor

glemieux commented Dec 22, 2023

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 🤷 ? I did reference is in a review comment, and some of the code from here appears in 2 stream, so maybe that's why?

@glemieux glemieux dismissed their stale review December 22, 2023 00:46

Somehow this review was submitted here even though I submitted it for a different PR. Very weird.

@rgknox
Copy link
Contributor Author

rgknox commented Jan 26, 2024

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.

@rgknox rgknox removed the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Jan 26, 2024
@rgknox
Copy link
Contributor Author

rgknox commented Jan 26, 2024

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.

Copy link
Contributor

@glemieux glemieux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I think this will further help orient developers to this large module when adding new output and trying to determine where things should be placed.

Most of my comments are related to things that have been commented out that might need cleanup or clarification.

main/FatesHistoryInterfaceMod.F90 Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesIOVariableKindMod.F90 Show resolved Hide resolved
@samsrabin
Copy link
Contributor

Concern raised by @ekluzek that "density" can be confusing. E.g., does this refer to single vs. double precision?

@rgknox
Copy link
Contributor Author

rgknox commented Mar 13, 2024

@samsrabin I updated language to use "dimension" instead of density

@rgknox
Copy link
Contributor Author

rgknox commented Mar 13, 2024

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

@rgknox rgknox merged commit 5e39a38 into NGEET:main Mar 13, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants