Updates and fixes to maintenance respiration #138

rgknox · 2016-10-14T18:17:16Z

This change group includes some unit fixes to calculations of maintenance respiration (sapwood and fine-root). Some refactoring was included with the change to make code more readable and organized. Several constants, such as unit conversions and physical constants were given named parameters. Constants that were general were added to GlobalConstants, local constants such as the base maintenance respiration rate were kept within the photosynthesis module. The code changes that generate different answers are related to fixing the unit conversion bug, associating fine-root c2n ratios with sapwood and fine roots, and also associating soil temperatures with the q10 temperature scaling on the respiration rates for below ground pools.

The changes should have an impact on plant carbon balance. Fine root respiration rates will now be a good deal larger than previously. Sapwood respiration rates, while larger, are still relatively small contributors compared to leaf (based on dark rates) and fine-roots, based on what is presumably a underestimated sapwood biomass pool.

Fixes: #120
User interface changes?: no
Code review: self, see #120 for discussions

Test suite: edTest, lawrencium-lr2
Test baseline: NA
Test namelist changes:
Test answer changes: yes, expected on FATES
Test summary: all pass

…sting, compiles.

…action

…ion to be consistent with the other maintenance pools. However, the unit of kgC/plant/sec is likely to be a very very small number that is wasting precision. Contemplating changing units to umol/sec or kgC/yr.

…thesis

…nce been used to calculate and update livecrootn, but the calculation had been moved to EDCanopyStructure. To resolve the conflict I had to remove the calculation of livecrootn and livestemn from its new home.

…d. I removed it.

…xed a unit conversion error in maintenance respiration.

… that was unnecessary, a scalar was sufficient. It was also calculated twice in the call sequence, where the first calculation was unnecessary and unused.

bandre-ucar · 2016-10-17T20:07:47Z

Seems like there is still some ongoing science discussion in #120. Please assign this to me when science review is done.

rgknox · 2016-10-17T20:35:33Z

Sounds good, thanks @bandre-ucar

rgknox · 2016-10-17T21:06:31Z

components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90

+                     ! the sapwood pools.
+                     ! Units are in (kgN/plant)
+                     ! ------------------------------------------------------------------
+                     live_agstem_n = ED_val_ag_biomass * currentCohort%bsw / &


After talking with @ckoven , changing the naming convention of "stem" to imply above ground, and use coarse root to imply below ground. New variable names will be:

live_stem_n
live_croot_n
froot_n

rgknox · 2016-10-18T20:46:38Z

components/clm/src/ED/main/HistoryIOMod.F90

@@ -692,6 +711,11 @@ subroutine update_history_prod(this,nc,nsites,sites,dt_tstep)

               if ( .not. ccohort%isnew ) then

+                  ! Calculate index for the scpf class
+                  ft  = ccohort%pft
+                  sc  = count(ccohort%dbh-sclass_ed.ge.0.0)


I think the calculation of sc is quite expensive, as it is doing some logical counting inside a cohort loop that occurs every time step.
I would like to add a new variable to the cohort structure that is calculated when dynamics are updated:

type(ed_cohort_type)
integer :: size_class ! An index that indicates which diameter size bin the cohort currently resides in
! this is used for history output. We maintain this in the main cohort memory
! because we don't want to continually re-calculate the cohort's position when
! performing size diagnostics at high-frequency calls
integer :: size_by_pft_class ! An index that indicates the cohorts position of the joint size-class x functional
! type classification. We also maintain this in the main cohort memory
! because we don't want to continually re-calculate the cohort's position when
! performing size diagnostics at high-frequency calls

…ariables were added to reduce the number of times these are re-calculated. They are in some cases needed for history diagnostics updated at the finest model time-scale on cohort variables, this was likely a very expensive calculation, so it was moved to be calculated only on the dynamics timestep.

rgknox · 2016-10-19T04:53:30Z

Tested the re-named variables and addition of the new indexing variables, edTest suite, 44d762d with regressions against fe9f7a1. All tests passed.

rgknox · 2016-10-24T18:36:25Z

@bandre-ucar , pending final agreement from @rosiealice, this should be ok to test on yellowstone. @rosiealice reviewed prior to my final analysis with updated root fractions. @rosiealice would you like more review on this, or any concerns you would like rectified? FYI, this change set is not a roadblock on other developments, so the time-line is open.

rosiealice · 2016-10-25T18:31:40Z

No, the modified root fractions is good with me. Yadvinder's paper has
leaf respiration being twice as large as root at Manaus & Tapajos, so this
might even be a real thing...
Table 13
https://www.researchgate.net/profile/Yadvinder_Malhi/publication/227546900_
Comprehensive_assessment_of_carbon_productivity_allocation_and_storage_in_
three_Amazonian_forests._Glob_Change_Biol/links/0fcfd5091b425a8e17000000.pdf

On 24 October 2016 at 12:36, Ryan Knox [email protected] wrote:

@bandre-ucar https://github.com/bandre-ucar , pending final agreement
from @rosiealice https://github.com/rosiealice, this should be ok to
test on yellowstone. @rosiealice https://github.com/rosiealice reviewed
prior to my final analysis with updated root fractions. @rosiealice
https://github.com/rosiealice would you like more review on this, or
any concerns you would like rectified? FYI, this change set is not a
roadblock on other developments, so the time-line is open.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#138 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AMWsQ75MBD3MwqopJSJqg4zhPrCok4Ohks5q3PqqgaJpZM4KXTBF
.

Dr Rosie A. Fisher

Terrestrial Sciences Section
Climate and Global Dynamics
National Center for Atmospheric Research
1850 Table Mesa Drive
Boulder, Colorado, 80305
USA.
+1 303-497-1706

http://www.cgd.ucar.edu/staff/rfisher/

bandre-ucar · 2016-10-27T18:32:54Z

components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90

@@ -979,7 +1012,7 @@ real(r8) :: tc                                ! Temperature response function fo
            end if
            bc_out(s)%rssun_pa(ifp) = rscanopy
            bc_out(s)%rssha_pa(ifp) = rscanopy
-            bc_out(s)%gccanopy_pa(ifp)  = 1.0_r8/rscanopy*cf/1000 !convert into umol m02 s-1 then mmol m-2 s-1. 
+            bc_out(s)%gccanopy_pa(ifp)  = 1.0_r8/rscanopy*cf/1000.0 !convert into umol m-2 s-1 then mmol m-2 s-1. 


This magic number needs an _r8 added. Shouldn't one of the new unit conversion constants be used instead?

added umol_per_mmol to FatesConstants

bandre-ucar · 2016-10-27T18:35:48Z

components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90

+
+    ! First guess on ratio between intracellular co2 and the atmosphere
+    ! an iterator converges on actual
+    real(r8),parameter :: init_a2l_co2_c3 = 0.7  


Need an _r8 on these constants.

bandre-ucar · 2016-10-27T18:39:09Z

components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90

    use EDCanopyStructureMod,only: calc_areaindex
+
+    use FatesConstantsMod, only : umolC_to_kgC,   &    ! micromole conversion to kgC


CLM has been actively discouraging these kinds of line continuations for several years because they make it hard to search the code for places where different names are imported. The preferred method is not using continuations and having the use statement on every line. Multiple names per line are ok.

It would be good for FATES to adopt a standard and start being consistent. I don't have a preference for what the standard is.

I like that standard, it will certainly help in tracking down where globals are used when grepping the code

bandre-ucar · 2016-10-27T18:42:31Z

components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90

+                                  g_per_kg,       &    ! number of grams per kg
+                                  mg_per_g,       &    ! number of miligrams per g
+                                  sec_per_min,    &    ! seconds per minute (60!)
+                                  rgas,           &    ! universal gas constant


Similar to tfrz, rgas should be named universal_gas_constant_UNITS and then aliased.

bandre-ucar · 2016-10-27T18:42:33Z

components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90

+
+    use FatesConstantsMod, only : umolC_to_kgC,   &    ! micromole conversion to kgC
+                                  g_per_kg,       &    ! number of grams per kg
+                                  mg_per_g,       &    ! number of miligrams per g


These comments aren't really contributing anything to understanding of the code. The point of using good naming conventions like this is that it removes the need for lots of explanatory comments.

ok, but if I come across a constant in the future that has a long or complex name, I might just provide the description in-line. I just want to brace you for that impact, I don't want to catch you off guard! These are important decisions.

bandre-ucar · 2016-10-27T18:44:00Z

components/clm/src/ED/biogeochem/EDCohortDynamicsMod.F90

@@ -316,6 +319,8 @@ subroutine nan_cohort(cc_p)
    currentCohort%canopy_layer       = 999 ! canopy status of cohort (1 = canopy, 2 = understorey, etc.)   
    currentCohort%NV                 = 999 ! Number of leaf layers: -
    currentCohort%status_coh         = 999 ! growth status of plant  (2 = leaves on , 1 = leaves off)
+    currentCohort%size_class         = 999 ! size class index


These magic numbers should be replaced with a parameter.

ok, I grabbed your definition of fates_unset_int from the history PR, I will use that, we should expect a trivial merge conflict with either this or History refactors, which every gets pulled second.

I'm going to leave the nan initialization in that subroutine as is. I will be investigating that part of the code anyway while I'm applying the fix to #133.

I'm not concerned about the conflict, it's easy enough to resolve. There are going to be a few conflicts with the hist mod anyway. At a minimum the set_hist_var calls need to be updated.

bandre-ucar · 2016-10-27T18:44:48Z

components/clm/src/ED/biogeochem/EDCohortDynamicsMod.F90

+    integer,intent(out) :: size_class
+    integer,intent(out) :: size_by_pft_class
+
+    size_class        = count(dbh-sclass_ed.ge.0.0)


Need an _r8.

… name, adding _r8 to local constants, adding an unset integer to global constants, removing some text that explained a constant.

rgknox · 2016-10-27T22:59:36Z

I believe comments by @bandre-ucar have been addressed now.
I would be amazed if something has been broken, but stranger things have happened. I will run it through the test mill and report in tomorrow.

bandre-ucar · 2016-10-28T17:21:15Z

testing on yellowstone

rgknox · 2016-10-28T18:16:12Z

The last changeset is not b4b with previous changesets and should be. I regressed 8e2b86f against fe9f7a1 and generated large differences across many variables.
One possible source is the swapping from a fates_unset_int flag of 999 to -9999 for a list of cohort variables (this would indicate one of those variables is not being initialized correctly).
Another source may be the conversion of some of those constants to double precision.
I'm running a regression tests where I revert that unset flag back to 999.

rgknox · 2016-10-28T19:31:55Z

I tried a couple of changes and re-ran the regression test: the following produced b4b results:

[rgknox@n0002 ed-clm]$ git diff
diff --git a/components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90 b/components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90
index a9e6cf5..f20bd6c 100644
--- a/components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90
+++ b/components/clm/src/ED/biogeophys/EDPhotosynthesisMod.F90
@@ -236,8 +236,8 @@ contains

     ! First guess on ratio between intracellular co2 and the atmosphere
     ! an iterator converges on actual
-    real(r8),parameter :: init_a2l_co2_c3 = 0.7_r8
-    real(r8),parameter :: init_a2l_co2_c4 = 0.4_r8
+    real(r8),parameter :: init_a2l_co2_c3 = 0.7 !_r8
+    real(r8),parameter :: init_a2l_co2_c4 = 0.4 !_r8


     associate(                                                &
@@ -1011,7 +1011,7 @@ contains
             end if
             bc_out(s)%rssun_pa(ifp) = rscanopy
             bc_out(s)%rssha_pa(ifp) = rscanopy
-            bc_out(s)%gccanopy_pa(ifp)  = 1.0_r8/rscanopy*cf/umol_per_mmol  !convert into umol m-2 s-1 then mmol m-2 s-1. 
+            bc_out(s)%gccanopy_pa(ifp)  = 1.0_r8/rscanopy*cf/1000.0 !umol_per_mmol  !convert into umol m-2 s-1 then mmol m-2 s-1. 
          end if

          currentPatch => currentPatch%younger

The non-b4b results can be attributed to rounding errors associated with the conversion of some single to double precision variables. I am satisfied with the results of the tests and recommend pulling pending tests on yellowstone.

EDIT: Note I don't want to revert the changes, I just wanted to know exactly why we didn't have b4b results on the last changeset.

ckoven · 2016-10-28T19:48:51Z

nice work, and, for the record, that's crazy that the single to double precision affects results so much.

bandre-ucar · 2016-10-28T19:52:04Z

@rgknox Thanks for following up with that. yellowstone tests are stuck in the queue. I'll push to master or post problems as soon as they are done.

rgknox and others added 10 commits October 7, 2016 16:52

Science fixes on sapwood and fine root respiration. First pass, no te…

32dd172

…sting, compiles.

fixed typo where MR of bg sapwood was double multiplied by bg root fr…

58e27de

…action

Added several AR diagnostics. Also changed the units of dark-respirat…

4416814

…ion to be consistent with the other maintenance pools. However, the unit of kgC/plant/sec is likely to be a very very small number that is wasting precision. Contemplating changing units to umol/sec or kgC/yr.

minor fix to update the pft index during the scpf output for photosyn…

0ca158e

…thesis

Merge resolution. EDCLMLinkMod was removed in master. That file had o…

a3e1966

…nce been used to calculate and update livecrootn, but the calculation had been moved to EDCanopyStructure. To resolve the conflict I had to remove the calculation of livecrootn and livestemn from its new home.

minor fix, livecrootn was still in a print statement that I overlooke…

4ecda62

…d. I removed it.

fixed hard-coded pft index in grperc found in photosynthesis

d0150fe

Merge branch 'andre-fates-constants' into rgknox-ckoven-froot-mr

f34bcb8

added some more global constants and uses in photosynthesis. Also, fi…

15824a2

…xed a unit conversion error in maintenance respiration.

intercellular co2 in photosynthesis was being represented as an array…

fe9f7a1

… that was unnecessary, a scalar was sufficient. It was also calculated twice in the call sequence, where the first calculation was unnecessary and unused.

rgknox changed the title ~~Updates and fixes to maintenance respiratoin~~ Updates and fixes to maintenance respiration Oct 14, 2016

rgknox commented Oct 17, 2016

View reviewed changes

renamed some local variables related to nitrogen content.

fb3e9ca

rgknox commented Oct 18, 2016

View reviewed changes

ckoven mentioned this pull request Oct 20, 2016

bug in froot_mr calculation #120

Closed

rgknox assigned bandre-ucar Oct 26, 2016

bandre-ucar reviewed Oct 27, 2016

View reviewed changes

Various syntactical fixes including: making rgas more explicit in its…

8e2b86f

… name, adding _r8 to local constants, adding an unset integer to global constants, removing some text that explained a constant.

bandre-ucar merged commit 8e2b86f into NGEET:master Oct 31, 2016

jenniferholm mentioned this pull request Jan 17, 2017

Fixes to carbon accounting and lr2 machine files #174

Merged

bandre-ucar removed their assignment Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates and fixes to maintenance respiration #138

Updates and fixes to maintenance respiration #138

rgknox commented Oct 14, 2016

bandre-ucar commented Oct 17, 2016

rgknox commented Oct 17, 2016

rgknox Oct 17, 2016

rgknox Oct 18, 2016

rgknox commented Oct 19, 2016

rgknox commented Oct 24, 2016

rosiealice commented Oct 25, 2016

bandre-ucar Oct 27, 2016

rgknox Oct 27, 2016

bandre-ucar Oct 27, 2016

rgknox Oct 27, 2016

bandre-ucar Oct 27, 2016 •

edited

Loading

rgknox Oct 27, 2016

bandre-ucar Oct 27, 2016

bandre-ucar Oct 27, 2016

rgknox Oct 27, 2016

bandre-ucar Oct 27, 2016

rgknox Oct 27, 2016

bandre-ucar Oct 27, 2016

bandre-ucar Oct 27, 2016

rgknox commented Oct 27, 2016

bandre-ucar commented Oct 28, 2016

rgknox commented Oct 28, 2016

rgknox commented Oct 28, 2016 •

edited

Loading

ckoven commented Oct 28, 2016

bandre-ucar commented Oct 28, 2016

		use EDCanopyStructureMod,only: calc_areaindex

		use FatesConstantsMod, only : umolC_to_kgC, & ! micromole conversion to kgC

Updates and fixes to maintenance respiration #138

Updates and fixes to maintenance respiration #138

Conversation

rgknox commented Oct 14, 2016

bandre-ucar commented Oct 17, 2016

rgknox commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgknox commented Oct 19, 2016

rgknox commented Oct 24, 2016

rosiealice commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bandre-ucar Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgknox commented Oct 27, 2016

bandre-ucar commented Oct 28, 2016

rgknox commented Oct 28, 2016

rgknox commented Oct 28, 2016 • edited Loading

ckoven commented Oct 28, 2016

bandre-ucar commented Oct 28, 2016

bandre-ucar Oct 27, 2016 •

edited

Loading

rgknox commented Oct 28, 2016 •

edited

Loading