Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incorrectly assuming at least canopy layer during promotion and demotion #1294

Open
rgknox opened this issue Dec 3, 2024 · 0 comments
Open

Comments

@rgknox
Copy link
Contributor

rgknox commented Dec 3, 2024

The algorithm in the canopy promotion/demotion code relies on the assumption that there is at least one canopy layer, particularly by setting this value to 1 (instead of 0) as the lower bound, see here:

https://github.com/NGEET/fates/blob/sci.1.80.3_api.37.0.0/biogeochem/EDCanopyStructureMod.F90#L2303

I don't know if this is creating any spurious results, but I suspect it is not. But, nonetheless, this is not correct. If there are no cohorts, this routine should report 0 canopy layers.

When I tried initializing z to 0 (on that line), I generated errors here: https://github.com/NGEET/fates/blob/sci.1.80.3_api.37.0.0/biogeochem/EDCanopyStructureMod.F90#L189-L191

I think this error triggered, because newly recruited cohorts were being added into the patches lowest canopy layer, (which is defined as patch%NCL_p), but that value was zero if the recruitment was applied to a patch with no existing cohorts.

Again, I don't think this is a big issue, but I wanted to document my thoughts in case it comes up. Here is a set of changes I would apply if I wanted to have the number of potential canopy layers (rightfully) return a 0. Note that it also initializes the NCL_p of a patch to -1, which is more appropriate, because at the time of creation, the number of canopy layers should not be assumed, it is unknown.

diff --git a/biogeochem/EDPhysiologyMod.F90 b/biogeochem/EDPhysiologyMod.F90
index 9f0db5ff..25e2e519 100644
--- a/biogeochem/EDPhysiologyMod.F90
+++ b/biogeochem/EDPhysiologyMod.F90
@@ -2542,7 +2542,7 @@ contains
             height             = EDPftvarcon_inst%hgt_min(ft)
             stem_drop_fraction = prt_params%phen_stem_drop_fraction(ft)
             fnrt_drop_fraction = prt_params%phen_fnrt_drop_fraction(ft)
-            l2fr               = currentSite%rec_l2fr(ft, currentPatch%NCL_p)
+            l2fr               = currentSite%rec_l2fr(ft, max(1,currentPatch%NCL_p))
             crowndamage        = 1 ! new recruits are undamaged
 
             ! calculate DBH from initial height 
@@ -2774,7 +2774,7 @@ contains
                call create_cohort(currentSite, currentPatch, ft, cohort_n,     &
                   height, 0.0_r8, dbh, prt, efleaf_coh, effnrt_coh, efstem_coh,  &
                   leaf_status, recruitstatus, init_recruit_trim, 0.0_r8,       &
-                  currentPatch%NCL_p, crowndamage, currentSite%spread, bc_in)
+                  max(1,currentPatch%NCL_p), crowndamage, currentSite%spread, bc_in)
 
                ! Note that if hydraulics is on, the number of cohorts may have
                ! changed due to hydraulic constraints.
diff --git a/biogeochem/FatesPatchMod.F90 b/biogeochem/FatesPatchMod.F90
index f8afc711..24298f81 100644
--- a/biogeochem/FatesPatchMod.F90
+++ b/biogeochem/FatesPatchMod.F90
@@ -737,7 +737,7 @@ module FatesPatchMod
 
       this%tr_soil_dir(:) = 1.0_r8
       this%tr_soil_dif(:) = 1.0_r8
-      this%NCL_p          = 1
+      this%NCL_p          = -1
 
       this%changed_landuse_this_ts = .false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ❕Todo
Development

No branches or pull requests

1 participant