-
Notifications
You must be signed in to change notification settings - Fork 343
Add history outputs: Crop biomass and LAI per harvest #3633
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
base: b4b-dev
Are you sure you want to change the base?
Conversation
Was using repr_grainc_to_food_patch + repr_grainc_to_seed_patch, but those don't get updated until after the harvest in CropPhenology, which is where the last CropPhaseTransitionBiomass() is called. They also, obviously, only include grain. Now using reproductivec_patch.
|
Adding "next" label until we get a reviewer. |
|
@slevis-lmwg can you take this one one, I think you have the most familiarity with the crop model. Is it helpful to go through the PR with @samsrabin ? |
slevis-lmwg
left a comment
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.
@samsrabin thank you for going over this work with me. I'm interested in looking again when you resolve the comment that you posted: https://github.com/ESCOMP/CTSM/pull/3633/changes#r2677039517
Also we agreed that this is fine in b4b-dev because it changes field-lists and not answers.
src/biogeochem/CNPhenologyMod.F90
Outdated
|
|
||
| else if (do_harvest) then | ||
| cphase(p) = cphase_harvest | ||
| call crop_inst%CropPhaseTransitionBiomass(p, cnveg_carbonstate_inst) |
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.
In other phases you include
if (cphase(p) /= cphase_orig) then before calling CropPhaseTransitionBiomass.
Here do you not because harvest occurs in one tstep always?
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.
More like: By definition, if we're harvesting, then it wasn't in cphase_harvest at the beginning of the subroutine.
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.
Another way of saying this: do_harvest means "cphase(p) isn't cphase_harvest but now it should be."
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.
I recommend a simple comment in the code to explain that.
| call restartvar(ncid=ncid, flag=flag, varname='frootc_maturity_thisyr_patch', xtype=ncd_double, & | ||
| dim1name='pft', dim2name='mxharvests', switchdim=.true., & | ||
| long_name='Fine root C at maturity for harvests in this patch this year', units='unitless', & | ||
| scale_by_thickness=.false., & | ||
| interpinic_flag='interp', readvar=readvar, data=this%frootc_maturity_thisyr_patch) | ||
| call restartvar(ncid=ncid, flag=flag, varname='livecrootc_maturity_thisyr_patch', xtype=ncd_double, & | ||
| dim1name='pft', dim2name='mxharvests', switchdim=.true., & | ||
| long_name='Live coarse root C at maturity for harvests in this patch this year', units='unitless', & | ||
| scale_by_thickness=.false., & | ||
| interpinic_flag='interp', readvar=readvar, data=this%livecrootc_maturity_thisyr_patch) | ||
| call restartvar(ncid=ncid, flag=flag, varname='livestemc_maturity_thisyr_patch', xtype=ncd_double, & | ||
| dim1name='pft', dim2name='mxharvests', switchdim=.true., & | ||
| long_name='Live stem C at maturity for harvests in this patch this year', units='unitless', & | ||
| scale_by_thickness=.false., & | ||
| interpinic_flag='interp', readvar=readvar, data=this%livestemc_maturity_thisyr_patch) | ||
| call restartvar(ncid=ncid, flag=flag, varname='leafc_maturity_thisyr_patch', xtype=ncd_double, & | ||
| dim1name='pft', dim2name='mxharvests', switchdim=.true., & | ||
| long_name='Leaf C at maturity for harvests in this patch this year', units='unitless', & | ||
| scale_by_thickness=.false., & | ||
| interpinic_flag='interp', readvar=readvar, data=this%leafc_maturity_thisyr_patch) | ||
| call restartvar(ncid=ncid, flag=flag, varname='reprc_maturity_thisyr_patch', xtype=ncd_double, & | ||
| dim1name='pft', dim2name='mxharvests', switchdim=.true., & | ||
| long_name='Reproductive C at maturity for harvests in this patch this year', units='unitless', & | ||
| scale_by_thickness=.false., & | ||
| interpinic_flag='interp', readvar=readvar, data=this%reprc_maturity_thisyr_patch) |
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.
should these have been "harvest" rather than maturity?
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.
For now I will wait for successful aux_clm testing before I hit "approve" if you don't mind @samsrabin
Description of changes
Adds a bunch of history outputs with the
mxharvestsdimension (max number of crop harvests per year; 2).MAX_TLAI_PERHARVgives the maximum total LAI seen across the growing season.20 other outputs give the biomass for five C pools (leaf, grain, stem, fine root, coarse root) at three crop phase transitions (emergence, anthesis, maturity) and harvest (which may not happen at maturity).
Also adds an option to suppress a warning about low
gddmaturity, which I used during the CRU-JRA crop calendar work.Specific notes
Contributors other than yourself, if any: None
CTSM Issues Fixed (include github issue #): None
Are answers expected to change (and if so in what way)? No
Any User Interface Changes (namelist or namelist defaults changes)? No
Does this create a need to change or add documentation? Did you do so? Will need to update non-FATES output list; no.
Testing performed
✅ aux_clm
Resolved with a7e66aa.aux_clmreveals what seem to be threading problems withMAX_TLAI_PERHARV, which failsCOMPARE_base_restinER[PS]_Ly3_P64x2tests but not anERS_Ly3test.✅ Nonsense check: Correct relative number of negative biomass pool outputs
The biomass pool outputs should be negative when a valid harvest doesn't occur (i.e., we're harvesting, and not a "fake harvest" or when the crop doesn't make it to the given phase transition.
AT_HARVESTvalues should never be negative when a valid harvest occurs.AT_ANTHESISvalues should always be ≥ the number of negativeAT_EMERGENCEvalues.This check passes in my CUPiD notebook for case
clm6_crops_omni05_unsimgeneric, run with a version up-to-date with commit 319be76:To do
Add HARVORGANC equivalent but JUST for grain. Will match what I have there for now, because grain is all CLM simulates by default. But it'll differ with the options underneath. (Do not name GRAIN unless you fix LPREPRSTRUCT test to handle this.)People should just use existingGRAINC_TO_(FOOD|SEED)_PERHARVoutputs.Add support forSee above.for_testing_use_second_grain_pooland/orfor_testing_use_repr_structure_pooltrue (or throw error in that case).