-
Notifications
You must be signed in to change notification settings - Fork 352
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
Enable flexible definitions of plant functional types (PFTs) #6433
Enable flexible definitions of plant functional types (PFTs) #6433
Conversation
This PR is passing all e3sm_land_developer tests on chrysalis against master baselines at hash dd32f0. |
There is a circleci: build failure for this PR, but I'm not able to interpret the details. It looks like a timeout problem. |
The new feature documentation page for this PR is on the Land group space on Confluence, at: |
@bishtgautam @bbye @rgknox - I'm hoping you can take a look at this PR and give me any feedback. Beth, this is the modification for flexible number and order of PFTs that I discussed earlier with you and Eva. Ryan, glad to get together to give you additional background if needed. This is part of the E3SM - NGEE Arctic coordination Epic. Thanks, Peter |
ok, I'll take a look @thorntonpe |
@bbye please review. |
@rgknox Based on our code review discussion on June 12, the ORNL NGEE Arctic group is planning to clean up the conditional deallocation / allocation at lines 297-306 in elm_initializeMod.F90, which also gets rid of a hard-coded mxpft_nc /= 24 reference. The new commit for those changes will be ready on this PR early next week. We also discussed with you some confusing use of numpft, mxpft, maxpatch_pft variables in elm_varpar.F90 and elsewhere. After giving that more careful review, we think this is something that is best tackled as an E3SM/land group code cleanup issue. The NGEE Arctic team will open an issue related to that with some suggestions about how the E3SM team can modify the use of these variables. |
@@ -672,7 +672,7 @@ subroutine Allocation1_PlantNPDemand (bounds, num_soilc, filter_soilc, num_soilp | |||
|
|||
f5 = 0._r8 ! continued intializations from above | |||
|
|||
if (ivt(p) >= npcropmin) then ! skip 2 generic crops | |||
if (iscft(ivt(p)) >= 1) then ! skip 2 generic crops |
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 we replace "1" with a named constant that conveys the meaning of 1 here?
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.
@rgknox there are a number of flags that are currently reals but are intended as logical (usually named is). In the spirit of getting this set up for a broader overhaul later on, I used iscft as an example and did the conversion throughout the code to a logical variable in the latest commit. That allows these kinds of tests to be of the form if (iscft(ivt(p))) then ..., or if (.not. iscft(ivt(p))) then ...
That seems like a clear approach for the logical flags. The use of multiple values for a flag like woody is going to take some additional thought and work, since it is sometimes used like if (woody > 1), which does not lend itself any better to a named value replacing the 1.
Hopefully this demonstration with iscft shows an acceptable path for future development, while allowing this PR to move ahead.
@@ -2748,7 +2748,7 @@ subroutine Allocation3_PlantCNPAlloc (bounds , & | |||
npool_to_frootn(p) = cpool_to_frootc(p) / cnfr | |||
npool_to_frootn_storage(p) = cpool_to_frootc_storage(p) / cnfr | |||
|
|||
if (woody(ivt(p)) == 1._r8) then | |||
if (woody(ivt(p)) >= 1.0_r8) then |
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.
is woody a real?
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.
Yes.
@@ -2563,8 +2563,7 @@ subroutine ch4_aere (bounds, & | |||
is_vegetated = .false. | |||
end if | |||
|
|||
if (veg_pp%itype(p) == nc3_arctic_grass .or. crop(veg_pp%itype(p)) == 1 .or. & | |||
veg_pp%itype(p) == nc3_nonarctic_grass .or. veg_pp%itype(p) == nc4_grass) then | |||
if (graminoid(veg_pp%itype(p)) == 1 .or. crop(veg_pp%itype(p)) == 1) then |
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.
same as comments related to comparisons with woody, can we compare this to a named constant instead of 1? Even if this is a integer acting as a logical, or a true logical, its better to compare to a named consant, like int_true or int_false.
@@ -1078,7 +1078,7 @@ subroutine Summary(this, bounds, num_soilc, filter_soilc, num_soilp, filter_soil | |||
! !USES: | |||
use elm_varpar , only: nlevdecomp,ndecomp_cascade_transitions,ndecomp_pools | |||
use subgridAveMod , only: p2c | |||
use pftvarcon , only : npcropmin | |||
use pftvarcon , only : iscft |
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 can be removed since its not used, right?
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.
Yes - removed in latest commit.
@@ -1893,13 +1894,17 @@ subroutine CropPhenologyInit(bounds) | |||
! Convert planting dates into julian day | |||
minplantjday(:,:) = huge(1) | |||
maxplantjday(:,:) = huge(1) | |||
do n = npcropmin, npcropmax |
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 see, so this is an example of added flexibility to where cfts are positioned in the list, we no longer rely on looping through the expected cft bounds, but go through all fts and check their iscft label...
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.
Is it possible to remove the usage of npcropmin and npcropmax altogether, and remove it from pftvarcon?
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.
That is something we can try to tackle in later PRs, focusing on code cleanup.
@@ -459,7 +459,7 @@ subroutine InitCold(this, bounds, ratio, c12_carbonstate_vars) | |||
if (veg_vp%evergreen(veg_pp%itype(p)) == 1._r8) then | |||
this%leafc_patch(p) = 1._r8 * ratio | |||
this%leafc_storage_patch(p) = 0._r8 | |||
else if (veg_pp%itype(p) >= npcropmin) then ! prognostic crop types | |||
else if (iscft(veg_pp%itype(p)) >= 1) then ! prognostic crop types |
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.
the definition of iscft in pftvarcon only covers two types 0 and 1, is this ">=" prepping for a future condition? If the meaning of iscft expands to be non-binary, we should update the description in pftvarcon when this happens.
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.
Nope, this was intended as binary. Now converted to that throughout the code. Thanks for the suggestion.
@@ -114,7 +110,7 @@ subroutine InitAllocate(this, bounds) | |||
! !LOCAL VARIABLES: | |||
integer :: i, imeg | |||
integer :: class_num | |||
real(r8) :: factors(numpft) | |||
real(r8) :: factors(numveg) |
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.
is this a bug fix? The allocation space on this changed, but its usage did not.
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.
Full range was not used previously.
@@ -869,7 +869,8 @@ subroutine CanopyFluxes(bounds, num_nolakeurbanp, filter_nolakeurbanp, & | |||
p = filterp(f) | |||
c = veg_pp%column(p) | |||
if(.not.veg_pp%is_fates(p)) then | |||
if (veg_pp%itype(p) == nsoybean .or. veg_pp%itype(p) == nsoybeanirrig) then | |||
! soybean (crop with N fixation) | |||
if (crop(veg_pp%itype(p)) >= 1 .and. nfixer(veg_pp%itype(p)) == 1) then |
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.
crop is still defined as a binary in pftvarcon:
E3SM/components/elm/src/main/pftvarcon.F90
Line 106 in 6ced7eb
real(r8), allocatable :: crop(:) !crop pft: 0. = not crop, 1. = crop pft |
Can you update the definition since this can be larger than 1?
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.
No plan for this to be greater than 1. This can be changed following the example of iscft, but would prefer to get it in a later PR.
@@ -48,20 +48,23 @@ module elm_varpar | |||
integer, parameter :: ndst = 4 ! number of dust size classes (BGC only) | |||
integer, parameter :: dst_src_nbr = 3 ! number of size distns in src soil (BGC only) | |||
integer, parameter :: sz_nbr = 200 ! number of sub-grid bins in large bin of dust size distribution (BGC only) | |||
integer, parameter :: mxpft = 50 ! maximum number of PFT's for any mode; | |||
!integer, parameter :: mxpft = 50 ! maximum number of PFT's for any mode; | |||
integer :: mxpft = 50 ! maximum number of PFT's for any mode; |
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.
shouldn't maxpft be a parameter constant?
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.
mxpft is now updated by reading the pft physiology file.
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.
Do all the physiology files need to be updated to read mxpft?
! FIX(RF,032414) might we set some of these automatically from reading pft-physiology? | ||
integer, parameter :: numveg = 16 ! number of veg types (without specific crop) | ||
integer, parameter :: nlayer = 3 ! number of VIC soil layer --Added by AWang | ||
integer :: nlayert ! number of VIC soil layer + 3 lower thermal layers | ||
|
||
integer :: numpft = mxpft ! actual # of patches (without bare), a total that spans LUs | ||
integer :: numpft = 50 ! actual # of patches (without bare), a total that spans LUs |
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.
Might be safer to initialize this to a nonsense number (a negative perhaps?), so that it crashes if there is some weird scenario where it does not get set, is it interpreted from reading in real data, right?
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.
mxpft is set by reading in real data. numpft is still used as an upper limit. We are working out how to simplify this but I'd like to do it in a later PR - it is a long-standing tangle of usage that this PR makes a little better, but doesn't go all the way to cleaning up.
@@ -123,34 +123,35 @@ subroutine convert_cft_to_pft( begg, endg, cftsize, wt_cft ) | |||
! a crop landunit, and put them on the vegetated landunit. | |||
! !USES: |
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 was grepping through the code to see where we use this routine convert_cft_to_pft, to see if the changes would effect FATES somehow. But I don't see where this is called. Is it actually called anywhere?
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.
It isn't called anywhere in the current code. It is a placeholder for future functionality.
I gave a look through the code, somewhere between "thorough" and "almost thorough". I think I came up with some reasonable (and perhaps nit-picky) things to look over. In general, my take is that the code uses too many "magic numbers", ie comparing variables to hard-coded constants like "1" or "0". I feel it is fair to say that fixing that type of thing is out of scope for this PR, but it could be an opportunity to make things more transparent if you agree with me. In general though, this PR is getting the code more flexible and robust since it breaks away from expecting an order to certain indices. Awesome stuff all. |
I added some comments that I saw during my review. A couple other things I wanted to bring up. 1) it might be nice for @evasinha to look at the PR since there are a lot of changes to the crop indexing and she knows more about how perennial crops are handled. 2) the PR should be tested with the crop model configuration (including a test with 2-way irrigation using generic crops) to make sure it doesn't break anything. 3) I noticed that iscft is either 0 or 1, but most of the if statements used >=1. I think this is ok since it gives us freedom to later expand the crops a bit. I'm thinking we could have annual crops=1 and perennial crops=2. I'm not asking for that change in this PR, but keeping consistent use of >=1 for iscft would allow us to change that in the future. |
|
||
! NOTE: there is a misunderstanding in pft physiology parameter file: 'perennial crop' IS NOT 'crop', | ||
! but still counted into 'numcft' | ||
if(percrop(i)==1) then |
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.
Does this mean that percrop has an iscft ==0?
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.
No, percrop and iscft are independent.
!if (elmveg == nc3crop ) wesveg = 2 | ||
!if (elmveg == nc3irrig ) wesveg = 2 | ||
!if (elmveg >= npcropmin .and. elmveg <= npcropmax ) wesveg = 2 | ||
if (crop(elmveg) == 1.0_r8 .or. iscft(elmveg) == 1.0_r8 ) wesveg = 2 |
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.
Here and a few other places iscft ==1 but almost everywhere else, the use is iscft>=1? I think it would be good to be consistent with the notation.
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.
Thanks -based on this and Ryan's comments I converted the iscft to a logical type in the latest commit.
@@ -305,11 +305,12 @@ subroutine FireArea (bounds, & | |||
if (pi <= col_pp%npfts(c)) then | |||
p = col_pp%pfti(c) + pi - 1 | |||
! For crop veg types | |||
if( veg_pp%itype(p) > nc4_grass )then | |||
if( crop(veg_pp%itype(p)) == 1 .or. iscft(veg_pp%itype(p)) == 1 )then |
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 location where == is used instead of >= for iscft. Also see lines 335 and 511 in this 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.
I would like to go through and do the conversion to logical types for a couple of other variables, including crop(), in the same way the I have done for iscft(). I would like to hold off and bring that in with other code cleanup in another PR.
get_map_EF = vocemis_vars%efisop_grc(5,g_in, ti_in) | ||
else if (ivt_in >= nc3crop) then !crops | ||
else if (crop(ivt_in) == 1 .or. iscft(ivt_in) == 1) then !crops |
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.
uses == instead of >= for iscft
@@ -869,7 +869,8 @@ subroutine CanopyFluxes(bounds, num_nolakeurbanp, filter_nolakeurbanp, & | |||
p = filterp(f) | |||
c = veg_pp%column(p) | |||
if(.not.veg_pp%is_fates(p)) then | |||
if (veg_pp%itype(p) == nsoybean .or. veg_pp%itype(p) == nsoybeanirrig) then | |||
! soybean (crop with N fixation) | |||
if (crop(veg_pp%itype(p)) >= 1 .and. nfixer(veg_pp%itype(p)) == 1) then |
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'm a little worried about this change. This code is specific to soybean since it is drought tolerant. I worry if using an nfixer flag any future crops that are classified as nfixers would inadvertently be included in this call. Currently we don't have any other nfixers, but I don't know if that will change.
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 point. This suggests that what we want is a drought-tolerant flag for crops, or better yet a change to the photosynthesis parameters controlling btran. Let's put this on our list for testing and changes in a future PR.
else if ( (max(iscft(i),crop(i)+percrop(i))) >= 1 .and. (nonvascular(i)+woody(i)+graminoid(i)) >= 1) then | ||
print *, 'ERROR: Incorrect crop PFT flags: ', i, ' ', trim(pftname(i)) | ||
call endrun(msg=' ERROR: crop PFT cannot be any of nonvascular/woody/graminoid type - '//errMsg(__FILE__, __LINE__)) | ||
end if |
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.
We have some woody crops targeted for future versions of the model, so we might want more flexibility with the flags.
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 error checks are very specific for the current usage of woody etc. I agree something more flexible would be good. Let's work on that together as a future PR.
done, thanks @thorntonpe |
@bbye please re-review. |
I only had one comment/question about whether we need to update all the physiology files since mxpft is now being read in. I know this is backward compatible, I just wasn't sure about that. Otherwise, looks fine to merge. |
@bbye Thanks for the further review. The mxpft is being calculated by reading the entries in the file, and there is no requirement to add it as a variable or modify the existing physiology files. |
Introduce a new optional format for PFT physiology files (paramfile) that allows user to specify a flexible number of PFTs, including additional natural vegetation types and different numbers of crops. Backwards compatible (BFB) with older style files. New variables added in new format file: - climatezone (options are: 0 {nonspecific}, 1 {tropical}, 2 {temperate}, 3 {boreal}, 4 {arctic}) - needleleaf (options are: 0, 1) - nfixer (options are: 0, 1) - nonvascular (options are: 0 {vascular}, 1 {moss}, 2 {lichen}) - graminoid (options are: 0 {non-grass}, 1 {grass}) - iscft (options are: 0 {natural PFT}, 1 {prognostic crop functional type}) Variables with expanded options in new format file: - woody (options are: 0{non-woody}, 1 {tree}, 2 {shrub}) Contributors: Code development: Fengming Yuan (yuanf@ornl.gov) Code development: Ben Sulman (sulmanbn@ornl.gov) [BFB]
…codes, but NOT YET fully work. Two tests are actually similary as 'elm_usrdat' test, but with 42_FLUXNETSITES, so those 2 shall produce same outputs. The 3rd one, usrpft_arctic_I1850CNPRDCTCBC, is for testing an actual Arctic Site (See Sulman et al. 2021), however which would not produce same results as the paper because ELM developments in arctic PFT physiology not yet included. NOTE: there are a few needed inputs are included in each tests, which if not in $DIN_LOC_ROOT user can copy them into to allow it work.
NOTES for when user-defined: (1) explicitly define a few PFT properties (flags) as following: nonvasular (0, 1=moss, or 2=lichen); woody (0, 1=tree, or 2=shrub); needleleaf ( 0=broadleaf, or 1=needleleaf); graminoid ( 0=woody/crop/nonvascular, or 1=graminoid); generic_crop ( 0=non-crop/prognostic crop, 1=generic_crop. NOTE that this is for not having conflicts with crop module development). climatezone (0=not-specific, 1=tropical, 2=temperate, 3=boreal, or 4=arctic) (2) in case's env_run.xml, add ' -maxpft 12' in which 12 is an example number of pfts, in "ELM_BLDNML_OPTS'" like the following: <entry id="ELM_BLDNML_OPTS" value="-maxpft 12 -bgc bgc -nutrient cnp -nutrient_comp_pathway rd -soil_decomp ctc -methane -nitrif_denitrif"> <type>char</type> <desc>CLM build-namelist options</desc> </entry>
…veg_pp. Only difference may be actual number of PFTs due to surface data 'natpft' length less than physiology parameter length ('npft'). So it's shall be exactly same by 'use pftvarcon, only: ', or by 'veg_pp%'. This commit try to use either in one code file of .F90.
…when flexible length pfts are user-provided, and/or surfdata 'lsmpft' dimension is less than 'pft' dimension in parameter file.
…ndexing may be not in need anymore.
This is a demonstration of how the same approach could be used for other flags that are used as logicals but allocated as reals or integers. [BFB]
4d622ce
to
0f841e6
Compare
…t (PR #6433) Introduce a new optional format for PFT physiology files (paramfile) that allows user to specify a flexible number of PFTs, including additional natural vegetation types and different numbers of crops. Backwards compatible (BFB) with older style files. New variables added in new format file: - climatezone (options are: 0 {nonspecific}, 1 {tropical}, 2 {temperate}, 3 {boreal}, 4 {arctic}) - needleleaf (options are: 0, 1) - nfixer (options are: 0, 1) - nonvascular (options are: 0 {vascular}, 1 {moss}, 2 {lichen}) - graminoid (options are: 0 {non-grass}, 1 {grass}) - iscft (options are: 0 {natural PFT}, 1 {prognostic crop functional type}) Variables with expanded options in new format file: - woody (options are: 0{non-woody}, 1 {tree}, 2 {shrub}) Thanks-to: Fengming Yuan (yuanf@ornl.gov), Ben Sulman (sulmanbn@ornl.gov) [BFB]
Introduce a new optional format for PFT physiology files (paramfile) that allows user to specify a flexible number of PFTs, including additional natural vegetation types and different numbers of crops. Backwards compatible (BFB) with older style files. New variables added in new format file: climatezone (options are: 0 {nonspecific}, 1 {tropical}, 2 {temperate}, 3 {boreal}, 4 {arctic}) needleleaf (options are: 0, 1) nfixer (options are: 0, 1) nonvascular (options are: 0 {vascular}, 1 {moss}, 2 {lichen}) graminoid (options are: 0 {non-grass}, 1 {grass}) iscft (options are: 0 {natural PFT}, 1 {prognostic crop functional type}) Variables with expanded options in new format file: woody (options are: 0{non-woody}, 1 {tree}, 2 {shrub}) Add 2 new tests for the user pft. ERS.ELM_USRDAT.I1850CNPRDCTCBC.elm-usrpft_default_I1850CNPRDCTCBC ERS.ELM_USRDAT.I1850CNPRDCTCBC.elm-usrpft_codetest_I1850CNPRDCTCBC Thanks-to: Fengming Yuan (yuanf@ornl.gov), Ben Sulman (sulmanbn@ornl.gov) [BFB]
Merged to next after being removed by a reset of next. |
Introduce a new optional format for PFT physiology files (paramfile) that
allows user to specify a flexible number of PFTs, including additional
natural vegetation types and different numbers of crops. Backwards compatible
(BFB) with older style files.
New variables added in new format file:
Variables with expanded options in new format file:
Add 2 new tests for the user pft.
ERS.ELM_USRDAT.I1850CNPRDCTCBC.elm-usrpft_default_I1850CNPRDCTCBC
ERS.ELM_USRDAT.I1850CNPRDCTCBC.elm-usrpft_codetest_I1850CNPRDCTCBC
Thanks-to: Fengming Yuan (yuanf@ornl.gov), Ben Sulman (sulmanbn@ornl.gov)
[BFB]