search

ESCOMP / CTSM

Community Terrestrial Systems Model (includes the Community Land Model of CESM)
http://www.cesm.ucar.edu/models/cesm2.0/land/
Other
301 stars 306 forks source link

flexibleCN and luna code cleanup #290

Open bandre-ucar opened 6 years ago

bandre-ucar commented 6 years ago

Migration of trello card. Some of this may have already been done or no longer relevant.

TODO list

Note From Dave Lawrence

These are notes that Charlie took based on meeting with Bardan. Could be helpful during the code cleanup.

notes, meeting with bardan jan. 26 2016

switches:

NutrientCompetitionFlexibleCNMod.F90

carbon_resp_opt: if C:N is too high, burn off excess or not.
downreg_opt: main flexcn switch. should really always be false when flexcn is activated, so we should remove thoe blocks of code in NutrientCompetitionFlexibleCNMod where that is not the case.
CN_residual_opt: only needed when cn_partition_opt is zero; if 1 then what do you do with excess residual nitrogen
CN_partition_opt: two options
CN_partition_opt == 0 is give all the plant parts other than the leaf N, and then give leaf the remaining N
CN_partition_opt == 1 (default) is distribute the N to all plant pools by relative demand

dynamic_plant_alloc_opt: not been tested; uses the friedlingstein 1999 allocation option. not tested, don't use. DELETE
nscalar_opt: if false, then doesn't limit the N uptake by C:N ratio; default is .TRUE.
plant_ndemand_opt: uptake of N only during day or day and night. 3 is default, means that plant N uptake occurs whenever plants have LAI
substrate_term_opt: if .FALSE., then no M-M uptake term; default is .TRUE. LEAVE THIS OPTION IN THE CODE FOR SURE, AS IT ALLOWS ISOLATING BARDAN’S LEAF-LEVEL CHANGES FROM HIS ROOT UPTAKE CHANGES

temp_scalar_opt: uses the soil temp scalar to limit N uptake if true. default is .TRUE. MAYBE THIS OUGHT TO BE TURNED OFF SINCE ITS EFFECT IS TOTALLY UNCLEAR?

CNPhenologyMod.F90

Bardan changed CN_evergreen_phenology_opt logic so that everything always goes through storage pool first
tranr=0.0002_r8 gives a residence time of a couple hours; means that now all the C and N flows through storage pool but quickly to display pool. MAKE THIS AN INPUT PARAMETER NOT A MAGIC NUMBER

PhotosynthesisMod.F90
vcmax_opt (defualt equals 3) these parameters are all the slopes and parameters from TRY
REDUNDANT PARAMETERS HERE: i_vca rather than i_vca. DELETE i_vca, i_vc, s_vca, s_vc FROM PARAMETER FILE AS THESE AREN'T USED BECAUSE VCMAX_OPT = 3 WHEN FLEXCN IS ON.
vcmax_opt == 0 SEEMS TO ASSUME THAT FLEXCN ISN'T ON. change to if .not. flexcn then do what is currently done when vcmax_opt == 0; and if flexcn == .TRUE. then do whatever happens when vcmax_opt == 3

list of magic numbers that ought to become input parameters:
tranr in CNPhenologyMod.F90 (.0002 1/s) i.e. very fast transfer out of storage pool
Kmin in NutrientCompetitionFlexibleCNMod.F90(1.0 gN / m^3)
Vmax_N in NutrientCompetitionFlexibleCNMod.F90 = 2.7E-8_r8 gN / gC
the +- 10 or +-15s used in the leafcn_min leafcn_max calc in NutrientCompetitionFlexibleCNMod.F90 ought to all be based on an uptake number and a burn-off number
e.g. change "frac_resp = (leafcn_actual - leafcn_max) / 10.0_r8" to "frac_resp = (leafcn_actual - leafcn_max) / 15.0_r8" and then make the 15 an input parameter (organ_CN_offoptimal_range_uptake and organ_CN_offoptimal_range_resp)

fr_leafn_to_litter (currently 50%, based on jackson review paper) ought to be a PFT-specific parameter

CNCStateUpdate1Mod.F90
All the code that is within the if (carbon_resp_opt == 1) logic is there to avoid the balance check error associated with double counting. REALLY THIS CODE SHOULDN'T BE IN THIS MODULE, AS THE POINT OF THIS MODULE IS TO BE THE EXPLICIT INTEGTRATION STEP RATHER THAN SETTING RATES. best to move to the allocation code for consistency.

Code review from Bill Sacks

Email from Bill Sacks:

Hi all,

Seeing all of the conditionally allocated LUNA variables throughout the code made me wonder if this could be handled more cleanly if LUNA had its own derived type / class, with possible polymorphism to handle the LUNA-on vs LUNA-off (i.e., old code) cases.

At a glance, it looks to me like this could lead to some significant cleanup of the code, although admittedly I have not done a careful analysis.

This definitely shouldn't be a priority before the science freeze, but I wanted to get these thoughts out there while they're fresh in my mind:

(1) Move the luna-specific variables in photosyns_type into luna_type.

Some of these (pnlc_z_patch, fpsn24_patch, enzs_z_patch) appear to only be used in LunaMod.

The others (vcmx25_z_patch, jmx25_z_patch) appear to be set in LunaMod, then used in PhotosynthesisMod.

To me, all of these are calling out to be moved into a new LunaType, which is local to LunaMod. Then this instance could be passed as input into Photosynthesis.

But at a glance, it looks like we could go much further than that: It looks (at a glance) like there is some code in subroutine Photosynthesis that is only needed for the luna-off case. Ideally, we'd have a LunaOffMod (with a better name) that computes vcmx25 and jmx25 using the old scheme, and then some of the code in PhotosynthesisMod that is specific to the old scheme could be moved in there – rather than cluttering up subroutine Photosynthesis with code that doesn't apply if you're using Luna (which makes it harder to understand what does and does not happen with Luna).

Update: Oh, I see that we fall back on using the old vcmx25 and jmx25 for c4 plants. This complicates things a bit, but I actually feel like this makes it even more important to have a clean design that isolates the luna-specific code, in order to (a) make the operation of luna easier to understand, and (b) make it less error-prone to (eventually) extend luna to add c4 plants.

One way this could be done is: Have a shared routine (in the base class used by both Luna and non-Luna) that computes vcmx25 and jmx25 in the old way. Then the non-Luna routine would simply look like:

call set_capacities_old_method(..., vcmx25(bounds%begp:bounds%endp), jmx25(bounds%begp:bounds%endp))

whereas the Luna code would look like:

call set_capacities_old_method(..., vcmx25_nonluna(bounds%begp:bounds%endp), jmx25_nonluna(bounds%begp:bounds%endp))

if (c3) then ! set vcmx25 and jmx25 based on luna method else vcmx25(bounds%begp:bounds%endp) = vcmx25_nonluna(bounds%begp:bounds%endp) jmx25(bounds%begp:bounds%endp) = jmx25_nonluna(bounds%begp:bounds%endp) end if

(2) It looks like the 3 new variables in atm2lndType and the 4 new variables in SolarAbsorbedType are simply accumulation variables, which are only used in LunaMod. These should simply be moved to the new LunaType, with accumulation code in LunaMod.

(3) I also see some other Luna-specific accumulation variables, such as in TemperatureType, that are not allocated in an "if use_luna" conditional, so which I missed in my first pass. These should also be moved into the new LunaType. I think we (or at least Mariana and I) decided in the past that it's best to have parameterization-specific accumulation variables in a parameterization-specific type/class. In general, it's better to have this modularity, even if it means a bit of duplication of these accumulation variables (if two parameterizations happen to need the same accumulation variable).

Based on this (quick) analysis, it looks to me like the fundamental responsibility of Luna is to calculate vcmx25 and jmx25 in a different way than what was done in the old code. Is that true? If so, I believe this proposed design would make this responsibility more explicit, thus making it easier for others to understand the code.

Bill

ekluzek commented 4 years ago

We are bringing these to the params file in the hardcodep branch: luna_theta_cj, jmaxb0, wc2wjb0, enzyme_turnover_daily, relhExp, minrelh