refactor code that assumes a particular ordering of PFT type constants

[billsacks]

Bill Sacks < sacks@ucar.edu > - 2013-12-23 12:57:11 -0700
Bugzilla 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.

[billsacks]

Erik Kluzek < erik@ucar.edu > - 2013-12-23 13:40:58 -0700

Hey Bill

I agree with you. This has been something I've been aware of as a danger, but haven't been able to refactor. I started adding the indices rather than having magic numbers hardcoded everywhere in CLM. That's part of the danger of this -- there are hard-coded assumptions about the values and likely still hard-coded magic numbers. I think we have a variable for tree, but we probably need something similar for shrub and grass.

I think I did add some limited checking that does verify things are expected when reading in the params file (pft con file). So if you change the order it might catch at least some situations. But, yeah it would be best if the code were flexible enough that the order didn't matter, especially as we are adding a ton of more crop PFT's.

[billsacks]

Joshua Rady has done some work towards improving this. His code and notes would be a good starting point.

[adrifoster]

This also seems like an interesting issue to tackle! @ekluzek and @billsacks is this something I could take a crack at?

[ekluzek]

@adrifoster this is something that would be good to do. But, it hasn't been a priority and we currently don't have a test to verify it. Since, we figure a test of this would fail -- it's not really something we've worried about. I guess the way to check it would be to have an alterative params file that changes the order, and then compare to the standard one and make sure it doesn't change answers. There might be some unit testing you could do as well.

I've thought of this as possibly being a really bit task and wondered how much worth it would have. But, possibly if you lay out some of this it might not be so bad? You could inspect the code for everyone the veg indices are queried and see how many instances there are. You would also want to look over pftconMod.F90 as that's where a lot of this is done. Also connecting with Joshua to see what he learned would be good as well. Even just documenting some of that here would be a good start.

So I think you could get into this if you like. If you'd also want to discuss more we could do that.

[billsacks]

I don't think this needs a test in the sense that @ekluzek seems to be thinking: I think the way to approach this would be to remove all checks involving numeric range comparisons (i.e., less than or greater than) with some pft type, replacing them with new flags like is_tree, is_grass, etc. Then as long as system tests still pass with that refactor, I'd say we have confidence that this is done correctly.

That said, I don't know what the priority of this is. I think that largely depends on whether this – or something like it – would be valuable for FATES runs, too. If not, then we could just live with this being not great for non-FATES configurations.