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

Variable VAI bin widths #752

Merged
merged 15 commits into from
Jan 15, 2022
Merged

Variable VAI bin widths #752

merged 15 commits into from
Jan 15, 2022

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Jun 17, 2021

This code allows for an exponential spacing to bin widths in the VAI (LAI+SAI) calculations. This should allow for higher resolution of the radiative transfer code at the top of the canopy where light levels are higher, but wider bins at the bottom of the canopy where light levels are lower. In principle this should do a few different things:

In discussions with @walkeranthonyp @rgknox @rosiealice we were looking at some different layering options in MAAT. @walkeranthonyp did some initial analyses in MAAT, and more needs to be done on that front to find a good layering that balances cost and accuracy, and to ensure that the same dynamics are at work in FATES.

In the current code, the number of leaf layers is hard-coded in the fortran, but the width of the top VAI bin, as well as the amount by which each subsequent bin is wider than the previous are both moved to the parameter file in order to facilitate experiments to determine what works best. The current default values basically keep the old layering scheme, by setting the width of the top bin width to 1 VAI unit and no change in bin thickness with depth.

fixes #492, since we've decided that a vertically-varying leaf layering system is preferable to a dynamic leaf-layering system.

This has some necessary but minimal HLM-side changes. Currently the CTSM branch is https://github.com/ESCOMP/CTSM/compare/master...ckoven:variable_dinc_2?expand=1 and it mainly adds the VAI bin edges to the history file metadata.

Will update this PR to include CTSM and ELM PRs once it is tested and gets closer to readiness.

Description:

Collaborators:

Expectation of Answer Changes:

Checklist:

  • My change requires a change to the documentation.
  • I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
  • I have read the CONTRIBUTING document.
  • FATES PASS/FAIL regression tests were run
  • If answers were expected to change, evaluation was performed and provided

Test Results:

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

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

FATES baseline hash-tag:

Test Output:

@ckoven ckoven added the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Jun 17, 2021
@rgknox
Copy link
Contributor

rgknox commented Jun 18, 2021

Since we plan on adding a Leaf photosynthetic temperature acclimation timescale parameter as well, it would be nice if these parameter file additions are all grouped together. Makes it easier on existing users to update their files..
Maybe we could migrate the new parameters defined here, as well as the API changes, to PR #724. Then, this PR would not require an API change and synchronization with ctsm/e3sm.

@rgknox rgknox mentioned this pull request Jun 21, 2021
@glemieux glemieux removed the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Sep 20, 2021
@@ -795,7 +799,16 @@ subroutine SetFatesGlobalElements(use_fates)
max_comp_per_site = 1
end if

! calculate the bin edges for radiative transfer calculations
Copy link
Contributor

Choose a reason for hiding this comment

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

@ckoven , while I feel this is as good a place as any, why did you decide to fill these arrays here? (maybe the answer is, "its as good a place as any...")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I couldn't think of a better place to do it?

@rgknox
Copy link
Contributor

rgknox commented Sep 21, 2021

@ckoven @rosiealice I see that these methods update currentPatch%elai_profile(L,ft,iv) and the like, which are the key arrays that drive the radiative transfer model. It seems, at a glance at least, that the radiative transfer model should be able to handle variable bin widths. Is this a definite, a probably, or something we should test further? I note that in the default parameter file, we maintain constant bin widths which seems like a good place to start.

@glemieux glemieux self-assigned this Oct 18, 2021
This tag is a bug fix. Changes prevented a logic check on hlm_pft_map from being applied when biogeography was not active
@glemieux glemieux added the ctsm An issue is related to ctsm host land model or a particular PR has a corresponding ctsm-side PR label Nov 2, 2021
@glemieux
Copy link
Contributor

glemieux commented Nov 3, 2021

Note: the ctsm-side update to remove the dinc_ed variable that will be made obsolete by this PR will be handled as part of ctsm5.1.dev062 tag: ESCOMP/CTSM#1540 (comment). The ctsm PR I created will be rolled into that tag. As such, we should wait to merge this PR until that tag has been posted to avoid breaking runs.

@glemieux
Copy link
Contributor

glemieux commented Nov 4, 2021

All expected tests PASS initial fates suite regression tests. There are four cases with DIFFs when compared against the baseline (all others are b4b):

  • SMS_Lm13.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_gnu.clm-FatesColdDef
  • SMS_Lm13.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdDef
  • SMS_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdDefReducedComplexSatPhen
  • SMS_Lm3_D_Mmpi-serial.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdDefHydro

The hydro run is resulting in only very small FATES_ERRH2O_SCPF errors. Both compiler versions of the 13 month sms tests start out with CBALANCE_ERROR_FATES and ERROR_FATES early in the run on the order of 10E-06 and then eventual start reporting other errors about 1.5 months into the run. The SatPhen sms test starts to diverge in C_STOMATA with modest differences about halfway through the first month and then starts to diverge by small differences in other variables soon after.

Results can be found here: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr752-vaibins

@ckoven
Copy link
Contributor Author

ckoven commented Dec 2, 2021

Updating this to note that I've compared the model dynamics with this and the current main branch and, while not bit-for-bit, the differences appear to be roundoff-level, since the new parameters that control for bin spacing are the same as in the old code. If I update the code to decrease the bin spacing, then differences do emerge as expected. So I think we should integrate this now, which would then allow us to explore the costs and benefits of different bin spacings as a future step.

@rosiealice
Copy link
Contributor

I am getting to the point where integrating this would be useful, fwiw...

This set of changes adds capabilities to track running means (both fixed window and ema) in fates. This requires some information on the model timestep from the HLM, and is thus an API change.  This also allows for the fusing and copying of these means at the cohort or patch level.
@glemieux
Copy link
Contributor

glemieux commented Dec 21, 2021

This PR now corresponds with ctsm PR ESCOMP/CTSM#1562. As a reminder, the original ctsm-side changes where brought in via ESCOMP/CTSM#1540.

@glemieux
Copy link
Contributor

glemieux commented Jan 12, 2022

Update: I've retested this after merging ctsm master into ESCOMP/CTSM#1562. The same subset of regression test are not b4b and have similar results as discussed in #752 (comment). Results can be found here:
/glade/u/home/glemieux/scratch/ctsm-tests/tests_vaibin-ctsmdev070-fates

As such, I think we will be good to integrate this once the aux_clm test for ESCOMP/CTSM#1562 is complete (which is underway).

Exact restart fix for fates satellite phenology mode
@glemieux
Copy link
Contributor

Final re-test after merging #821 into this PR:

Izumi:
All expected fates suite tests pass B4B except for:
SMS_Lm3_D_Mmpi-serial.1x1_brazil.I2000Clm50FatesCruRsGs.izumi_nag.clm-FatesColdDefHydro
which is an expected DIFF per the comment discussions above.
Test location: /scratch/cluster/glemieux/ctsm-tests/tests_vai-bin-fates-ersfix

Cheyenne:
The set of DIFFs has expanded slightly to include the other hydro tests with the same order of magnitude errors, so this is acceptable in my view. Additionally, now that the ERS sp mode test is running that is showing similar diffs to the SMS sp mode test. All other expected fates suite test pass B4B.
Test lcoation: /glade/u/home/glemieux/scratch/ctsm-tests/tests_vaibin-ctsmdev070-fates-ersfix

@glemieux glemieux merged commit 477a8d5 into NGEET:master Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ctsm An issue is related to ctsm host land model or a particular PR has a corresponding ctsm-side PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dynamic leaf layer depth system
4 participants