-
Notifications
You must be signed in to change notification settings - Fork 92
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
Initialize nocomp mode with large trees #995
Initialize nocomp mode with large trees #995
Conversation
This reverts commit 3d56c69.
Conflicts: main/EDInitMod.F90
main/EDInitMod.F90
Outdated
call bbgw_allom(temp_cohort%dbh,pft,c_bgw) | ||
! calculate crown area of a single plant | ||
dummy_n = 1.0_r8 ! make n=1 to get area of one tree | ||
spread = 1.0_r8 ! fix this to 0 to remove dynamics of canopy clousre, assuming a closed canopy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are these two named constants that you can optionally use for spread, if you like. Seems like we initialize with a spread of 0 for an inventory init, 1 for a bare ground init.
https://github.com/NGEET/fates/blob/sci.1.64.2_api.25.2.0/main/EDTypesMod.F90#L108-L109
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I should be using init_spread_inventory since this isn't a near bare ground case. I've changed it now.
main/EDInitMod.F90
Outdated
write(fates_log(),*) 'c_area: ', temp_cohort%c_area | ||
write(fates_log(),*) 'p_area: ', patch_in%area | ||
|
||
deallocate(temp_cohort) ! get rid of temporary cohort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a duplicate deallocate, will be harmless, but better to remove it because the following deallocation has more logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch, thanks. Deleted the duplicate.
main/EDInitMod.F90
Outdated
temp_cohort%coage, temp_cohort%dbh, prt_obj, cstatus, rstatus, & | ||
temp_cohort%canopy_trim, temp_cohort%c_area,1,temp_cohort%crowndamage, site_in%spread, bc_in) | ||
|
||
write(fates_log(),*) 'c_area: ', temp_cohort%c_area |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these look temporary, can remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
temp_cohort%canopy_trim = 1.0_r8 | ||
temp_cohort%crowndamage = 1 ! Assume no damage to begin with | ||
|
||
if(EDPftvarcon_inst%initd(pft)>nearzero) then ! interpret as initial density and calculate diameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JessicaNeedham Should we gracefully fail (during parameter reading) a cold-start simulation where initd=0 (ie abs(initd)<nearzero) ? It seems like if someone sets the diameter to zero, or the density to zero, the run will fail anyway. Although the user may be wondering what happened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could add a check here I think: https://github.com/NGEET/fates/blob/sci.1.64.2_api.25.2.0/main/EDPftvarcon.F90#L1531
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I've added a check in EDPftvarcon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks great to me. Thanks for adding the fail. I guess my only thing is that I would prefer having the dbh initialization be a separate parameter rather than used when initd
is negative, but it's not a super strong opinion.
Thanks for the reviews @rgknox and @adrifoster ! @adrifoster I agree that initial dbh value might be better as its own parameter. It's kind of confusing to have parameters mean different things depending on context. Unless anyone has strong feelings otherwise I will go ahead and add it as a new parameter. Couple of questions on how best to do this:
|
…ality of the initd parameter.
At the software meeting today we discussed whether to add another parameter v include better comments in the code to explain the dual function of the fates_recruit_init_density parameter. We decided on better comments so I have gone ahead and added those to EDInitMod. |
Regression testing on Cheyenne underway |
…zes' into JFN-nocomp-init-large-sizes
All expected tests are B4B against the Folder on Cheyenne: |
Description:
This PR allows nocomp mode to be initialised with large trees by allowing the initial_density parameter to be interpreted as initial dbh when negative. It then calculates the crown area of a single tree using that DBH, and calculates number density needed to fill the canopy.
Related to issue #994 and discussion #985
Collaborators:
@ckoven @jenniferholm @rgknox @glemieux
Expectation of Answer Changes:
This PR will change answers only in nocomp mode when the parameter fates_recruit_init_density is set to a negative number.
Checklist:
Test Results:
CTSM (or) E3SM (specify which) test hash-tag:
CTSM (or) E3SM (specify which) baseline hash-tag:
FATES baseline hash-tag:
Test Output: