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

Added three new history variable dimensions #179

Merged
merged 19 commits into from
Feb 3, 2017
Merged

Added three new history variable dimensions #179

merged 19 commits into from
Feb 3, 2017

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Jan 20, 2017

Added three new history variable dimensions: size, age, and PFT.

Previously you could add variables on a multiplexed size x PFT dimension, but not just size, and just PFT was using the levsoi dimension instead of its own dedicated dimension. Also there was no way to output things binned by patch-age, and the multiplexed dimension is a bit unwieldy such that none of those variables are default-on. Since the dimension definitions straddle the FATES-HLM interface, fixing these will be trickier to change once the interface is in place; also we will soon want the mxpft that sets the length of the PFT dimension to be a runtime-set parameter, so separating it from nlevsoi removes a currently-implied dependency that mxpft must be less than nlevsoi. In addition to adding these dimensions, I also pulled all the old PFT-resolved variables off of the nlevsoi axis and onto the new PFT axis, and added some new variables on the size and age axes. These include a default-on basal area by size class diagnostic, and some new diagnostics about the patch age distribution, npp, gpp, and canopy and leaf areas resolved by patch age.

Fixes: #145

User interface changes?: No

Code review: went over most of the code with @rgknox

Test suite: ed test suite on lawrencium-lr3, intel
Test baseline: a5dc8da
Test namelist changes: n/a
Test answer changes: bit for bit

Test summary: ALL PASS

@ckoven
Copy link
Contributor Author

ckoven commented Jan 20, 2017

hearing no objections, and since this is a b4b PR, I'm assigning this to @bandre-ucar to test on yellowstone etc.

@@ -1169,6 +1170,7 @@ subroutine fuse_2_patches(dp, rp)

!area weighted average of ages & litter
rp%age = (dp%age * dp%area + rp%age * rp%area)/(dp%area + rp%area)
rp%age_class = max(1,count(rp%age-ageclass_ed.ge.0.0_r8))
Copy link
Contributor

Choose a reason for hiding this comment

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

@ckoven, why is this max(1,...)? It seemed to me that once the age is fused, the age class should be calculated from that new age using the same method in all places. In ed_integrate_state_variables(), the calcuation has no max(1, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough, I was just erring on the side of safety. if you think there is no way that numerical weirdness could cause two combining patch ages to be less than zero, i can remove and retest.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh, my brain flipped, I was thinking min(1, ...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. but do you think i should leave as-is or take out the max? in principle it should never encounter an age less than zero and so the max ought to be redundant, but weirder things have happened.

Copy link
Contributor

Choose a reason for hiding this comment

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

if I had my druthers, I would say remove the max(1, ...) part, and create a single function that can be used in the different places. If the age is below zero, the model should crash gracefully. I wouldn't want the model to push-on and cover up these negatively aged patches. Thanks @ckoven

@rgknox
Copy link
Contributor

rgknox commented Jan 20, 2017

great changes @ckoven.
I read in your summary that the intention is to change the size of the pft output dimension from mxpft to a run-time set parameter. I am imagining that with the parameter refactors we will have a value that indicates the maximum number of fates PFTs that we get from the file. It may retain the name numpft_ed, but we will see.

We should make it clear from the history files that they are FATES pft's and fates dimensions, as we don't want users to confuse those pft dimensions with other HLM PFTs. I forget where that happens. Do we assign long-names to the dimensions?

We also might want to take this time to change the dimensions sent to the HLM code to use "fates" naming conventions instead of "ed".

@@ -2322,6 +2326,10 @@ subroutine htape_timeconst(t, mode)
long_name='pft index of the combined pft-size class dimension', units='-', ncid=nfid(t))
call ncd_defvar(varname='scls_levscpf',xtype=ncd_int, dim1name='levscpf', &
long_name='size index of the combined pft-size class dimension', units='-', ncid=nfid(t))
call ncd_defvar(varname='levage',xtype=tape(t)%ncprec, dim1name='levage', &
long_name='patch age (yr)', ncid=nfid(t))
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe indicate FATES age

call ncd_defvar(varname='levage',xtype=tape(t)%ncprec, dim1name='levage', &
long_name='patch age (yr)', ncid=nfid(t))
call ncd_defvar(varname='levpft',xtype=ncd_int, dim1name='levpft', &
long_name='pft number', ncid=nfid(t))
Copy link
Contributor

Choose a reason for hiding this comment

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

indicate "FATES" pft number

@ckoven
Copy link
Contributor Author

ckoven commented Jan 20, 2017

thanks @rgknox -- all good suggestions. i'll do these, retest on lawrencium, and push up a new change once tested.

@@ -1169,23 +1345,49 @@ subroutine define_history_vars(this, initialize_variables)

call this%set_history_var(vname='PFTbiomass', units='gC/m2', &
Copy link
Contributor Author

Choose a reason for hiding this comment

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

while at it, I'm also going to put these variable names as all-caps, since that follows style better, if that's okay with everyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, keeping these PFT variable names as-is to avoid testing issues

@rgknox
Copy link
Contributor

rgknox commented Jan 24, 2017

Changes look good to me, thanks for the work @ckoven.

@bandre-ucar
Copy link
Contributor

tests running on yellowstone and hobart.

@bandre-ucar bandre-ucar merged commit 043c03a into NGEET:master Feb 3, 2017
@bandre-ucar bandre-ucar removed their assignment Apr 6, 2018
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.

new history dimension: size class only
3 participants