ESCOMP / ESCOMP/CTSM

refactor code that assumes a particular ordering of PFT type constants

Open
#21 10 comments 0 reactions 0 assignees View on GitHub
code health good first issue
Dominant language
Fortran
Stars
352
Forks
361
Avg merge
2d 21h
Merged PRs (30d)
7

Description

**Bill Sacks < sacks@ucar.edu > - 2013-12-23 12:57:11 -0700**
**Bugzilla Id:** [1886](http://bugz.cgd.ucar.edu/show_bug.cgi?id=1886)
**Bugzilla CC:** bandre@lbl.gov, dlawren@ucar.edu, muszala@ucar.edu, mvertens@ucar.edu, rfisher@ucar.edu,

Here's a fun thought experiment: How much of CLM would break if you changed the numbering of these variables in pftvarcon:

integer :: noveg !value for not vegetated
integer :: ndllf_evr_tmp_tree !value for Needleleaf evergreen temperate tree
integer :: ndllf_evr_brl_tree !value for Needleleaf evergreen boreal tree
integer :: ndllf_dcd_brl_tree !value for Needleleaf deciduous boreal tree
integer :: nbrdlf_evr_trp_tree !value for Broadleaf evergreen tropical tree
integer :: nbrdlf_evr_tmp_tree !value for Broadleaf evergreen temperate tree
integer :: nbrdlf_dcd_trp_tree !value for Broadleaf deciduous tropical tree
integer :: nbrdlf_dcd_tmp_tree !value for Broadleaf deciduous temperate tree
integer :: nbrdlf_dcd_brl_tree !value for Broadleaf deciduous boreal tree
integer :: ntree !value for last type of tree
integer :: nbrdlf_evr_shrub !value for Broadleaf evergreen shrub
integer :: nbrdlf_dcd_tmp_shrub !value for Broadleaf deciduous temperate shrub
integer :: nbrdlf_dcd_brl_shrub !value for Broadleaf deciduous boreal shrub
integer :: nc3_arctic_grass !value for C3 arctic grass
integer :: nc3_nonarctic_grass !value for C3 non-arctic grass
integer :: nc4_grass !value for C4 grass

I don't know the answer to this question, but I know the answer is non-zero. Here are some examples, from a quick search:

From pftdynMod:

! If this is a tree pft, then
! get the annual harvest "mortality" rate (am) from harvest array
! and convert to rate per second
if (ivt(p) > noveg .and. ivt(p) < nbrdlf_evr_shrub) then

From CNFireMod:

! For crop veg types
if( pft%itype(p) > nc4_grass )then
cropf_col(c) = cropf_col(c) + pft%wtcol(p)
end if
! For natural vegetation (non-crop)
if( pft%itype(p) >= ndllf_evr_tmp_tree .and. pft%itype(p) <= nc4_grass )then
lfwt(c) = lfwt(c) + pft%wtcol(p)
end if

From CNVegStructUpdateMod:

! if shrubs have a squat taper
if (ivt(p) >= nbrdlf_evr_shrub .and. ivt(p) <= nbrdlf_dcd_brl_shrub) then
taper = 10._r8

And a corollary question: How much of CLM SHOULD break when you change the ordering of these? I would argue strongly that the answer to this question SHOULD BE ZERO.

The problems I see with usages like the above examples are:

(1) If we change the ordering of these pfts in the future, or remove any pfts, we'll break a lot of code

(2) More importantly: If a user naively tries to add a new pft, they are likely to inadvertently break code in hard-to-detect ways. For example, if someone naively added a new shrub type before nbrdlf_evr_shrub, it would be treated as a tree by the above code in pftdynMod.

What should be done? I'd say that the only conditional that should be allowed involving ivt is equality: no checks of less than something or greater than something. To enable logic like the above, there should be additional metadata associated with each pft, like is_tree, is_crop, is_natveg, is_shrub, etc.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.