E3SM-Project
Enable flexible definitions of plant functional types (PFTs)
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 ([email protected]), Ben Sulman ([email protected]) [BFB]
This PR is passing all e3sm_land_developer tests on chrysalis against master baselines at hash dd32f0. Two new tests are added to e3sm_land_developer suite. Those tests require a new paramfile and new datm7 files. I have copied those into the inputdata directory on chrysalis.
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: https://acme-climate.atlassian.net/wiki/spaces/LAND/pages/4335861769/Arctic+PFTs+NGEE+Arctic+IM4
@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
@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.
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.
Thanks @rgknox for giving this a careful look. I agree with your suggestions, and I'll discuss with @fmyuan and @bsulman. They are working on an additional commit, and we should be able to get at least some of your requested changes wrapped into this PR.
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.
@rgknox and @bbye - based on latest two commits, and responses to your reviews, I'm hoping this PR can now be approved.
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.