bugfix on understory demotion #271

rgknox · 2017-09-08T06:28:04Z

When the understory needed demotions (a situation that we had not been encountering until only recently), the accounting for the lost area had not been accounted for correctly. In short, when a cohort is demoted, its crown area has to be added to the layer below. But in the case of understory demotion, the cohorts should not be donating area to the layer below, because their is none, and they are generating litter instead.

Code review: pending

Test suite: cheyenne (ed, clm_short_45, clm_short_50), 300 year f45_f45 smoke test (by @jkshuman, see #267 PASS)

Test namelist changes: none
Test answer changes: none expected (should only expect answer changes if understory experiences demotion, nigh impossible within the first year)

Test summary (updated 9/13/2017):
cheyenne(intel): all PASS

…ost during demotion of the understory layer.

…ed nan checks to self equivalence.

jenniferholm · 2017-09-11T18:02:38Z

Hi @rgknox - I will get started on testing this issue and understory demotion.

I was having difficulty with successfully building this branch in ALM-FATES (https://github.com/rgknox/fates/tree/rgknox-test-layering), but this was on Thursday of last week. You might have made some changes since then, so should I checkout this branch again? Just double checking on the latest version.

Also - should I test in strict PPA?

jenniferholm · 2017-09-11T18:04:57Z

Hi @rgknox -- Here was my build error using your branch "rgknox-test-layering". This might be something ALM-FATES specific......

ERROR: Command: '/global/scratch/jaholm/ACME-fates-rgknox/components/clm/cime_config/buildnml /global/scratch/jaholm/Models/FATES_runs/ICLM45ED_ACME-FATES_f45_13pfts_layering' failed with error 'No handlers could be found for logger "CIME.utils"
No handlers could be found for logger "CIME.utils"
......
......
No handlers could be found for logger "CIME.utils"
** source directory does not exist: /global/scratch/jaholm/ACME-fates-rgknox/components/clm/src/external_models/mpp/src/mpp/dtypes
ERROR clm.buildnml: /global/scratch/jaholm/ACME-fates-rgknox/components/clm/bld/configure -comp_intf MCT -phys clm4_5 -usr_src /global/scratch/jaholm/Models/FATES_runs/ICLM45ED_ACME-FATES_f45_13pfts_layering/SourceMods/src.clm failed: 512'

rgknox · 2017-09-11T18:25:59Z

@jenniferholm : I'l bet that error can be fixed by "git submodule update --init --recursive"

…op in canopy_structure.

rgknox · 2017-09-13T23:52:14Z

Merged in the fixes that resolved #267. Re-testing on Cheyenne.

rosiealice · 2017-09-15T21:20:11Z

main/EDMainMod.F90

-    if ( abs(error) > 10e-6 ) then
+    ! We are not closing this error within 10e-6 very often
+    ! but this is filling up the logs too much
+    ! Encapsulating print statements and making new issue (RGK 09-11-2017)


Did you make a new issue? I should have made this an end run while it still passed the balance checks!

Thanks for the reminder. I didn't

I think it is #182

rosiealice · 2017-09-15T21:21:59Z

main/FatesConstantsMod.F90

@@ -61,10 +61,12 @@ module FatesConstantsMod
  ! Conversion: days per second
  real(fates_r8), parameter :: days_per_sec = 1.0_fates_r8/86400.0_fates_r8

-  ! Conversion: days per year. assume HLM uses 365 day calendar.  If we need to link to 365.25-day-calendared HLM, rewire to pass through interface
+  ! Conversion: days per year. assume HLM uses 365 day calendar.  
+  ! If we need to link to 365.25-day-calendared HLM, rewire to pass through interface


We had a discussion about usig Gregorian calendars in the CLM code meeting last week, pertaining to the inability to do so in CISM (I think!), so just wanted to flag that @billsacks has been giving this sort of thing a lot of thought and might have an opinion on what to do and not do.

@rosiealice is right about this discussion, though is giving me too much credit for how much thought I've given this: Basically, I decided to punt on this for CISM for now so I don't need to think about it 😄 That said, the idea proposed in this comment of passing the current year length through the interface seems reasonable. That's one of the approaches being considered for CISM. One complication here (at least for CISM) is that you can no longer treat this as a constant set in initialization or at compile-time, since year length will vary from year to year.

rosiealice · 2017-09-15T21:24:03Z

main/EDParamsMod.F90

-    call fates_params%RetreiveParameter(name=ED_name_grass_spread, &
-         data=ED_val_grass_spread)
-
-    call fates_params%RetreiveParameter(name=ED_name_comp_excln, &


Have these gone somewhere else? ED_val_comp_exlcn seems to still be used?

They were actually redundant. We defined each twice, and read each twice. I just removed the extra calls.

rosiealice · 2017-09-15T21:32:02Z

biogeochem/EDCanopyStructureMod.F90

+      do while (associated(currentPatch)) ! Patch loop    
+
+
+         ! Perform a numerical check on input data structures


Maybe make this section on nan checking it's own function. Seems like it might be useful elsewhere also?

hmm, ok, maybe a patch level function call that checks through all variables listed as arguments...

rosiealice · 2017-09-15T21:34:06Z

biogeochem/EDCanopyStructureMod.F90

+            do while(area_not_balanced)
+
+               ! ---------------------------------------------------------------------------------------
+               ! Demotion Phase: Identify upper layers that are too full, and demote them to


This is so much more zen than before.

rosiealice · 2017-09-15T21:37:41Z

biogeochem/EDCanopyStructureMod.F90

+         ! Not quite sure why we have this block of code, seems like a small subset of cases
+         ! Essentially promote all cohorts from layer below if that whole layer has area smaller
+         ! than the tolerance on the gains needed into current layer
+         ! -------------------------------------------------------------------------------------


I think it's so we don't break the carbon balance by losing this little set of cohorts. Suspect it's to do with 'trickling' in of recruits each day. I should know, obviously...

It just seems like the code below that block will serve the same function. Perhaps not as quickly, but that small lower layer will get completely promoted even without that first block of code right?
I don't think we need to remove it, as it does the task of promoting that specific case more efficiently.

Might the default promoting code end up with loads of tiny cohorts in the 'layer below'? This block could act as a 'terminate cohorts' type of thing, but for canopy layering?

I think we should keep the block and I will remove the comment.

rosiealice · 2017-09-15T21:38:36Z

biogeochem/EDCanopyStructureMod.F90

+         endif                 
+
+         sumdiff = 0.0_r8    
+         rankordered_area_so


Can we make this a namelist variable? I think the PFT file switches are pretty confusing for most people.

Wasn't sure exactly what you were pointing at, but it looks like you want the non-probabalistic exclusion switch as a NL variable right?

A switch instead of ED_val_comp_excln right?

Can we make this an issue so we can bat back and forth the pros and cons?

rosiealice · 2017-09-15T21:49:17Z

biogeochem/EDCanopyStructureMod.F90

+         endif                 
+
+         sumdiff = 0.0_r8    
+         rankordered_area_so


while i'm here, should we separate canopy_structure and the canopy_summarization&leaf_area_profile code into different files. I feel like they are awkward bedfellows, since they are trying to achieve quite orthogonal things...

rosiealice

I think this all looks great. It's much clearer to read overall than it was before.

rosiealice · 2017-09-15T21:52:08Z

biogeochem/EDCanopyStructureMod.F90

+         ! Not quite sure why we have this block of code, seems like a small subset of cases
+         ! Essentially promote all cohorts from layer below if that whole layer has area smaller
+         ! than the tolerance on the gains needed into current layer
+         ! -------------------------------------------------------------------------------------


Might the default promoting code end up with loads of tiny cohorts in the 'layer below'? This block could act as a 'terminate cohorts' type of thing, but for canopy layering?

rgknox · 2017-09-16T00:32:43Z

Added numerical checks (nan, overflow and underflow) as generic subroutines. These subroutines, if detecting numerical problems will dump site, patch and cohort information when possible, and exit.

Didn't remove the DEBUG bypass on the error check, but I see that is issue #182, and I will prioritize that issue (along with logging) next.

Removed that language where I was questioning if the block of code should had been kept.

Will re-test.

Any remaining items @rosiealice ?

rgknox · 2017-09-18T18:29:23Z

all tests re-run, all pass

rgknox · 2017-09-18T18:36:03Z

If no comments or concerns crop up by the end of the day I will merge this in.

rgknox added 5 commits September 5, 2017 13:32

Added debug flags to canopy structure code.

d72f1d8

Added more reporting and loop checking to canopy structure.

bd417ad

potential bug-fix to incorrect assesment of canopy areas gained and l…

949a66c

…ost during demotion of the understory layer.

Removed redundant parameter entries.

7b997f6

Removed ieee numerical library call, for compatibilty purposes. Chang…

9432dd5

…ed nan checks to self equivalence.

rgknox added 2 commits September 11, 2017 17:34

Wrapped demotion, fusion, promotion and fusion within one checking lo…

40bc7e8

…op in canopy_structure.

Put an oft warned message about a biomass check behind a debug wall.

800e502

rosiealice reviewed Sep 15, 2017

View reviewed changes

rosiealice approved these changes Sep 15, 2017

View reviewed changes

rgknox added 2 commits September 15, 2017 17:28

Added some functions for numerical checks.

df47cfa

Removed unnecessary comment.

9cb0d2e

rgknox merged commit c8e6da5 into NGEET:master Sep 19, 2017

jenniferholm mentioned this pull request Sep 23, 2017

Canopy too full; smaller top layer than bottom layer; hanging case #268

Closed

rgknox deleted the rgknox-test-layering branch July 31, 2018 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix on understory demotion #271

bugfix on understory demotion #271

rgknox commented Sep 8, 2017 •

edited

Loading

jenniferholm commented Sep 11, 2017

jenniferholm commented Sep 11, 2017

rgknox commented Sep 11, 2017

rgknox commented Sep 13, 2017

rosiealice Sep 15, 2017

rgknox Sep 15, 2017

rgknox Sep 15, 2017

rosiealice Sep 15, 2017

billsacks Sep 16, 2017

rosiealice Sep 15, 2017

rgknox Sep 15, 2017

rosiealice Sep 15, 2017

rgknox Sep 15, 2017

rosiealice Sep 15, 2017

rosiealice Sep 15, 2017

rgknox Sep 15, 2017

rosiealice Sep 15, 2017

rgknox Sep 15, 2017

rosiealice Sep 15, 2017

rgknox Sep 15, 2017

rosiealice Sep 15, 2017

rgknox Sep 15, 2017

rosiealice Sep 15, 2017

rosiealice Sep 15, 2017 •

edited

Loading

rosiealice left a comment

rosiealice Sep 15, 2017

rgknox commented Sep 16, 2017

rgknox commented Sep 18, 2017

rgknox commented Sep 18, 2017

		do while (associated(currentPatch)) ! Patch loop


		! Perform a numerical check on input data structures

bugfix on understory demotion #271

bugfix on understory demotion #271

Conversation

rgknox commented Sep 8, 2017 • edited Loading

jenniferholm commented Sep 11, 2017

jenniferholm commented Sep 11, 2017

rgknox commented Sep 11, 2017

rgknox commented Sep 13, 2017

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

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

Choose a reason for hiding this comment

rosiealice Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

rosiealice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgknox commented Sep 16, 2017

rgknox commented Sep 18, 2017

rgknox commented Sep 18, 2017

rgknox commented Sep 8, 2017 •

edited

Loading

rosiealice Sep 15, 2017 •

edited

Loading