Add some biophysical variables by land-use type#1407
Open
ckoven wants to merge 5 commits intoNGEET:mainfrom
Open
Add some biophysical variables by land-use type#1407ckoven wants to merge 5 commits intoNGEET:mainfrom
ckoven wants to merge 5 commits intoNGEET:mainfrom
Conversation
github-project-automation
bot
moved this to Finding Reviewers
in FATES Pull Request Planning and Status
ckoven
added
type: enhancement
software: history output
samsrabin
requested changes
May 13, 2025
Contributor
samsrabin
left a comment
samsrabin
left a comment
There was a problem hiding this comment.
Looks good, Charlie! Just a few suggestions.
Also, a question: I'm having trouble understanding why and under what circumstances these should fail to sum to the site value. Could you explain that in a bit more detail?
github-project-automation
bot
moved this from Finding Reviewers
to Under Review
in FATES Pull Request Planning and Status
Contributor
|
@ckoven where is this on the priority queue for the CLM6 code freeze? |
glemieux
added
the
software: API
Contributor
Author
|
added E3SM PR for this: E3SM-Project/E3SM#7783 |
5 tasks
glemieux
approved these changes
Feb 9, 2026
Contributor
|
@samsrabin would you confirm that your suggestions have been resolved and approve this if so? |
Contributor
|
GitHub PRs are seemingly having a bad day, so I'll work on this tomorrow. |
glemieux
reviewed
Feb 14, 2026
| ! =============================================================================================== | ||
|
|
||
|
|
||
| subroutine update_history_hifrq_landuse(this,nc,nsites,sites,bc_in,dt_tstep) |
Contributor
There was a problem hiding this comment.
Linking @rgknox comment here update_history_hifrq_landuse due to closing #1529
glemieux
reviewed
Feb 14, 2026
Comment on lines
+5427
to
+5429
| cpatch => sites(s)%oldest_patch | ||
| do while(associated(cpatch)) | ||
| if (cpatch%total_canopy_area .gt. rsnbl_math_prec) then |
Contributor
There was a problem hiding this comment.
Copying @rgknox question here from #1529 (comment)
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to look at biophysical effects of land use, it's useful to have a small number of relevant variables indexed by land use type. This PR allows that, so that you can look at: vegetation temperature, surface air temperature, and a basic set of energy fluxes (SW, LW, SH, LH) separately for each land use type. I.e., they are output on the
fates_levlandusehistory dimension for each gridcell.This requires HLM-side logic. I have a commit on E3SM that handles this at ckoven/E3SM@5fd137b (PR forthcoming)
Currently these are all output when the hi frequency history flag is set to 1; in principle this should actually be when the flag is set to 2, so that should probably change before bringing this PR in. (for the purpose of the experiments I am currently running, I want these and only these subgridscale high-frequency variables though.)
Also, the logic that averages in bare-ground-patch properties will only work for nocomp configurations. Further work is needed to be correct for full-FATES configurations as well, but I got lost in how to index a given site's bare-ground patch when nocomp isn't on, so I need to come back to that.
Also, the patch-weighting logic for these variables is slightly different than for other patch-level variables (e.g., the `fates_levage' -indexed ones). This is for two reasons: (1) we need to average in some of the bare ground patch properties to weight things correctly, because the FATES and HLM patch areas mean different things, and (2) the land use areas are less generally dynamic than the age binned logic, and so there is less chance of weirdness when the averaging denominator changes abruptly. So the upshot is that less postprocessing is needed for these (i.e they are simpler to work with), but also that they might not sum perfectly to the total site values because of changing denominators over time.
Description:
fixes #1404.
Collaborators:
discussed with @glemieux, @rgknox, @aswann
Expectation of Answer Changes:
This only adds new diagnostic variables, so should be bit for bit otherwise.
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
Integrator
If satellite phenology regressions are not b4b, please hold merge and notify the FATES development team.
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: