Refactor and standardize per-ageclass history #1252

samsrabin · 2024-09-16T21:50:30Z

Description:

As noted in Issue #1205, some per-ageclass history variables are weighted based on a patch's area (or a cohort's density) relative to age class area, whereas others are weighted based on that relative to total site area. To avoid confusion, this should be standardized. Discussion led us to decide on standardizing to the latter to avoid order-of-operations issues.

During development, I discovered and fixed two other bugs:

FATES_VEGC_APPF was incorrectly not being updated for new cohorts (contributes to Some biomass outputs only updated for non-new cohorts #1261).
FATES_MORTALITY_CANOPY_SZAP and FATES_MORTALITY_USTORY_SZAP were incorrectly excluding termination and logging mortality.

Supersedes PR #1225, where I had made the opposite change (standardizing all to use per-ageclass area).

Collaborators:

@ckoven, @rgknox

Expectation of Answer Changes:

These variables are changed to be weighted relative to site area (rather than age-class area):

FATES_GPP_AP
FATES_LAI_AP
FATES_NCL_AP
FATES_NPP_AP
FATES_SCORCH_HEIGHT_APPF

These variables are changed to be weighted relative to site-wide canopy area (rather than age-class canopy area):

FATES_LBLAYER_COND_AP
FATES_STOMATAL_COND_AP

These variables are affected by bug fixes:

FATES_MORTALITY_CANOPY_SZAP (logging and termination mortality now included)
FATES_MORTALITY_USTORY_SZAP (logging and termination mortality now included)
FATES_VEGC_APPF (see Some biomass outputs only updated for non-new cohorts #1261)

Other per-ageclass variables see roundoff-level changes as a result of refactoring (absolute RMSE < 1e-12, relative RMSE < 1e-13). In my testing (SMS_Lm49.f10_f10_mg37.I2000Clm60Fates.derecho_intel.clm-FatesColdAllVarsMonthly), the following were affected:

FATES_BURNFRAC_AP
FATES_DDBH_CANOPY_SZAP
FATES_DDBH_USTORY_SZAP
FATES_FIRE_INTENSITY_BURNFRAC_AP
FATES_FUEL_AMOUNT_AP
FATES_FUEL_AMOUNT_APFC
FATES_MORTALITY_CFLUX_CANOPY
FATES_MORTALITY_CFLUX_PF
FATES_MORTALITY_CFLUX_USTORY
FATES_NPLANT_CANOPY_SZAP
FATES_NPLANT_SZAP
FATES_NPLANT_SZAPPF
FATES_NPLANT_USTORY_SZAP
FATES_NPP_APPF
FATES_SCORCH_HEIGHT_PF
FATES_VEGC_AP

Answers from that test (at 1aff666) compared to baseline can be found here. Note that FATES_NPLANT_SZAPPF was not tested to see if it sums correctly across the age-class axis because I don't have a way to handle triplexed variables; however, it was only affected at roundoff level by this work.

Checklist

All checklist items must be checked to enable merging this pull request:

Contributor

The in-code documentation has been updated with descriptive comments
The documentation has been assessed to determine if updates are necessary: Yes

Integrator

FATES PASS/FAIL regression tests were run
Evaluation of test results for answer changes was performed and results provided

Documentation

Technical Note update:
User's Guide update:

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

The goal here is to simplify things and improve my understanding before more substantive refactoring and fixing. Changes: - Do everything within one patch loop, rather than looping through to get numerators and denominators separately, then looping again to do the division. - Use new "weight" variable instead of repeating (e.g.) "cpatch*area * AREA_INV". - Replace "hio_area_si_age(io_si, ipa2)*AREA" with "sites(s)%area_by_age(ipa2)" - Replace "sites(s)%area_by_age(cpatch%age_class)" with new "age_class_area" variable. Should be identical within roundoff precision. Affected variables: - FATES_BURNFRAC_AP - FATES_CANOPYAREA_AP - FATES_FIRE_INTENSITY_BURNFRAC_AP - FATES_FUEL_AMOUNT_AP - FATES_GPP_AP - FATES_LAI_AP - FATES_LBLAYER_COND_AP - FATES_NCL_AP - FATES_NPP_AP - FATES_SCORCH_HEIGHT_APPF - FATES_SECONDAREA_ANTHRODIST_AP - FATES_SECONDAREA_DIST_AP - FATES_STOMATAL_COND_AP - FATES_VEGC_AP - FATES_ZSTAR_AP Also renames various internal FatesHistoryInterfaceMod variables from "area" to "fracarea" for clarity (units are m2/m2, not m2).

To avoid similar mistakes in the future, adds function SumMortForHistory to fates_cohort_type.

These are useful for checking whether the per-ageclass versions are normalized correctly. * FATES_CANOPYAREA * FATES_NCL * FATES_PATCHAREA * FATES_SCORCH_HEIGHT_PF * FATES_SECONDAREA_ANTHRODIST * FATES_SECONDAREA_DIST * FATES_ZSTAR

Now dividing by total site area instead of age-class area: - FATES_GPP_AP - FATES_NPP_AP - FATES_LAI_AP - FATES_NCL_AP - FATES_SCORCH_HEIGHT_APPF Now dividing by site-wide canopy area instead of age-class canopy area: - FATES_LBLAYER_COND_AP - FATES_STOMATAL_COND_AP

See Issue NGEET#1261.

Tell the user how to get the value they probably want---the value on each age-class---rather than what each age-class contributes to the cross-ageclass area-weighted mean.

- Use descriptive weight names - Remove unused variable ipa - Organize into sections - Add a TODO

- Use descriptive weight names - Remove unused variable ipa - Fix indentation

main/FatesHistoryInterfaceMod.F90

samsrabin · 2024-10-16T18:18:53Z

main/FatesHistoryInterfaceMod.F90

@@ -358,7 +358,7 @@ module FatesHistoryInterfaceMod

  integer :: ih_primaryland_fusion_error_si

-  integer :: ih_area_si_landuse
+  integer :: ih_fracarea_si_landuse


I renamed a bunch of area variables to be fracarea instead, as "area" implies m2 but most of these are fractional. There are still some area variables in other subroutines, like ih_area_plant_si, that should probably also be changed for consistency.

I've now changed other such variables throughout this module to match.

main/FatesHistoryInterfaceMod.F90

samsrabin · 2024-10-21T20:00:53Z

Rename and add comments to update_history_dyn1() and update_history_dyn2() to indicate they're intended for site-level vs. multiplexed outputs, respectively.

biogeochem/FatesCohortMod.F90

rgknox · 2024-10-22T16:25:49Z

biogeochem/FatesCohortMod.F90

+      real(r8) :: mort_natural
+      real(r8) :: mort_logging
+      real(r8) :: mort_sum
+


Lets double check the units on the natural versus the logging mortality, I seem to remember they were different, such as their usage here:

https://github.com/NGEET/fates/blob/main/biogeochem/EDMortalityFunctionsMod.F90#L347-L354

Yep, that's what the per_year option is for—depending on its value, one will be converted into units of the other. I'll add units to comments in revision.

No, the problem is that one is in # individuals and the other is fraction.

Fix this.

~~@samsrabin here is an example of how to get total carbon biomass from the cohort, if you want to add it into this routine. This will help you convert from number of individuals to kgC:~~

~~https://github.com/NGEET/fates/blob/main/main/FatesHistoryInterfaceMod.F90#L3895-L3903~~

I looked at how this sum mortality was being used, and I had the units wrong, it looks like the function is being called with the expectation that the output units are not in carbon but the abundance rate.

Part of the confusion is related to our incorrect labeling of the rates on the cohort structures. Here is where we apply mortality to actually change number density, dndt is a fraction per time (because that fraction is multiplying by cohort%n:

https://github.com/NGEET/fates/blob/main/main/EDMainMod.F90#L804

https://github.com/NGEET/fates/blob/main/biogeochem/EDMortalityFunctionsMod.F90#L352-L354

…dyn1().

samsrabin · 2024-10-29T17:13:37Z

@rgknox @ckoven This is ready for re-review. The only remaining thing I need to ask about (marked by TODOs in the code) is that the reproductive biomass seems to not be included in total biomass in some parts of the history code, whereas it is included in others. Do you think there's a reason for that?

Used in history outputs to get biomass of each organ, plus alive and total biomass.

main/FatesHistoryInterfaceMod.F90

glemieux

Review part 1/2. Really love the work to break things up more here. There was one minor area of potential clean up that I wanted to have double checked.

glemieux · 2024-11-09T00:47:07Z

main/FatesHistoryInterfaceMod.F90

+                   iscag_anthrodist = get_age_class_index(cpatch%age_since_anthro_disturbance)

-                   hio_agesince_anthrodist_si_age(io_si,ageclass_since_anthrodist) = &
-                        hio_agesince_anthrodist_si_age(io_si,ageclass_since_anthrodist)  &
+                   hio_agesince_anthrodist_si(io_si) = &
+                        hio_agesince_anthrodist_si(io_si)  &


Since we're removing the second dimension here can we remove this call to get_age_class_index?

glemieux · 2024-11-09T00:59:12Z

main/FatesHistoryInterfaceMod.F90

+    real(r8) :: repro_m            ! Total reproductive mass (on plant) ""
+    real(r8) :: patch_area_div_site_area     ! Weighting based on patch area relative to site area
+    real(r8) :: patch_canarea_div_site_area  ! Weighting based on patch canopy area relative to site area
+    real(r8) :: cohort_n_div_site_area       ! Weighting based on cohort density relative to site area


FYI, I created a tasklist to track these sort of simple improvements for future PRs: #1282

glemieux

Review 2/2. I have one question on the hio_c_stomata_si_age calculation. I also have one minor suggested change to condense loops, but it can be delayed if necessary to move forward. Great work.

glemieux · 2024-11-11T18:20:51Z

main/FatesHistoryInterfaceMod.F90

+          do ft = 1,numpft
+             hio_scorch_height_si_pft(io_si,ft) = hio_scorch_height_si_pft(io_si,ft) + &
+                  cpatch%Scorch_ht(ft) * patch_area_div_site_area
+          end do
+
+          hio_ncl_si_age(io_si,cpatch%age_class) = hio_ncl_si_age(io_si,cpatch%age_class) &
+               + cpatch%ncl_p * patch_area_div_site_area
+          do ft = 1,numpft
+             iagepft = get_agepft_class_index(cpatch%age,ft)
+             hio_scorch_height_si_agepft(io_si,iagepft) = hio_scorch_height_si_agepft(io_si,iagepft) + &
+                  cpatch%Scorch_ht(ft) * patch_area_div_site_area
+          end do


Suggested change

do ft = 1,numpft

hio_scorch_height_si_pft(io_si,ft) = hio_scorch_height_si_pft(io_si,ft) + &

cpatch%Scorch_ht(ft) * patch_area_div_site_area

end do

hio_ncl_si_age(io_si,cpatch%age_class) = hio_ncl_si_age(io_si,cpatch%age_class) &

+ cpatch%ncl_p * patch_area_div_site_area

do ft = 1,numpft

iagepft = get_agepft_class_index(cpatch%age,ft)

hio_scorch_height_si_agepft(io_si,iagepft) = hio_scorch_height_si_agepft(io_si,iagepft) + &

cpatch%Scorch_ht(ft) * patch_area_div_site_area

end do

do ft = 1,numpft

iagepft = get_agepft_class_index(cpatch%age,ft)

hio_scorch_height_si_pft(io_si,ft) = hio_scorch_height_si_pft(io_si,ft) + &

cpatch%Scorch_ht(ft) * patch_area_div_site_area

hio_scorch_height_si_agepft(io_si,iagepft) = hio_scorch_height_si_agepft(io_si,iagepft) + &

cpatch%Scorch_ht(ft) * patch_area_div_site_area

end do

hio_ncl_si_age(io_si,cpatch%age_class) = hio_ncl_si_age(io_si,cpatch%age_class) &

+ cpatch%ncl_p * patch_area_div_site_area

Suggestion to condense numpft loops. Not critical.

glemieux · 2024-11-11T19:09:29Z

main/FatesHistoryInterfaceMod.F90

+          if (site_canopy_area .gt. nearzero) then
+             patch_canarea_div_site_canarea = cpatch%total_canopy_area / site_canopy_area
+             hio_c_stomata_si_age(io_si,cpatch%age_class) = &
+                  hio_c_stomata_si_age(io_si,cpatch%age_class) + &
+                  cpatch%c_stomata * mol_per_umol &
+                  * patch_canarea_div_site_canarea


Apologies if this was discussed elsewhere and I missed it. The previous version of this calculation was dividing through the sum of the total_canopy_area just for the given age class:

fates/main/FatesHistoryInterfaceMod.F90

Lines 5603 to 5606 in 1be3962

! Normalize resistance diagnostics

if (canopy_area_by_age(ipa2) .gt. nearzero) then

hio_c_stomata_si_age(io_si,ipa2) = &

hio_c_stomata_si_age(io_si,ipa2) / canopy_area_by_age(ipa2)

fates/main/FatesHistoryInterfaceMod.F90

Lines 5297 to 5298 in 1be3962

canopy_area_by_age(cpatch%age_class) = &

canopy_area_by_age(cpatch%age_class) + cpatch%total_canopy_area

This change appears to introduce dividing the by the site_canopy_area. Do I have that correct and is that the intent?

samsrabin added bug - software engineering PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. history output Pertaining to FATES history output variables refactor Restructures code without changing functionality labels Sep 16, 2024

samsrabin self-assigned this Sep 16, 2024

samsrabin mentioned this pull request Sep 16, 2024

Refactor and fix per-age-class history outputs #1225

Closed

4 tasks

samsrabin linked an issue Sep 16, 2024 that may be closed by this pull request

Incorrect (?) calculation of per-age-class outputs #1205

Open

samsrabin force-pushed the refactor-history-2 branch 2 times, most recently from fa87c89 to 1d1c44f Compare October 10, 2024 22:50

samsrabin mentioned this pull request Oct 11, 2024

Some biomass outputs only updated for non-new cohorts #1261

Open

samsrabin removed the refactor Restructures code without changing functionality label Oct 11, 2024

samsrabin added 8 commits October 11, 2024 07:41

Refactor: Age-class vars to update_history_hifrq2_ageclass().

3458eb6

Refactor: Age-class vars to update_history_dyn2_ageclass().

f66a99b

Bugfix: Add logging mortality to _SZAP outputs.

ae92beb

To avoid similar mistakes in the future, adds function SumMortForHistory to fates_cohort_type.

Add non-per-ageclass versions of some history vars.

9492573

These are useful for checking whether the per-ageclass versions are normalized correctly. * FATES_CANOPYAREA * FATES_NCL * FATES_PATCHAREA * FATES_SCORCH_HEIGHT_PF * FATES_SECONDAREA_ANTHRODIST * FATES_SECONDAREA_DIST * FATES_ZSTAR

Bugfix: Update FATES_VEGC_APPF even if cohort is new.

bf3684d

See Issue NGEET#1261.

Bugfix: Add termination mortality to _SZAP outputs.

7e1bf60

samsrabin force-pushed the refactor-history-2 branch from 1d1c44f to 7e1bf60 Compare October 11, 2024 13:41

samsrabin added 3 commits October 13, 2024 14:44

Add postprocessing info to per-ageclass outputs.

92ddeeb

Tell the user how to get the value they probably want---the value on each age-class---rather than what each age-class contributes to the cross-ageclass area-weighted mean.

Cleanup: update_history_dyn2_ageclass().

309086c

- Use descriptive weight names - Remove unused variable ipa - Organize into sections - Add a TODO

Cleanup: update_history_hifrq2_ageclass().

8356d40

- Use descriptive weight names - Remove unused variable ipa - Fix indentation

samsrabin force-pushed the refactor-history-2 branch from d249965 to 8356d40 Compare October 13, 2024 21:06

samsrabin marked this pull request as ready for review October 13, 2024 21:10

samsrabin changed the title ~~[WIP] Refactor and standardize per-ageclass history~~ Refactor and standardize per-ageclass history Oct 13, 2024

samsrabin commented Oct 14, 2024

View reviewed changes

main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved

samsrabin commented Oct 14, 2024

View reviewed changes

main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved

samsrabin commented Oct 14, 2024

View reviewed changes

main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved

samsrabin commented Oct 14, 2024

View reviewed changes

main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved

samsrabin commented Oct 16, 2024

View reviewed changes

samsrabin commented Oct 21, 2024

View reviewed changes

main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved

glemieux requested review from rgknox and ckoven October 21, 2024 19:17

rgknox reviewed Oct 22, 2024

View reviewed changes

biogeochem/FatesCohortMod.F90 Show resolved Hide resolved

rgknox reviewed Oct 22, 2024

View reviewed changes

biogeochem/FatesCohortMod.F90 Show resolved Hide resolved

rgknox reviewed Oct 22, 2024

View reviewed changes

samsrabin added 9 commits October 28, 2024 16:16

Move FATES_NCL from update_history_dyn2_ageclass() to update_history_…

7d5d8f9

…dyn1().

Add units info to SumMortForHistory().

e11f2fd

update_history subroutine names/descriptions now more descriptive.

4d22d79

update_history_dyn_subsite_ageclass(): Resolve a TODO.

ae499bf

Delete unused canopy_area_by_age.

9d9da65

Rename _area_ vars in FatesHistoryInterfaceMod to _fracarea_.

bcc15e7

Fix some indentation.

d967fdc

Resolve TODO about fire variable weighting.

c60f83b

Remove unneeded cohort class index updates.

f54db49

samsrabin requested a review from rgknox October 29, 2024 17:13

samsrabin added 2 commits November 6, 2024 11:28

Add prt_vartypes subroutine GetBiomass().

acb58bb

Used in history outputs to get biomass of each organ, plus alive and total biomass.

Add comment about reproductive mass being 0.

2242bd1

samsrabin requested a review from glemieux November 6, 2024 20:17

rgknox reviewed Nov 8, 2024

View reviewed changes

main/FatesHistoryInterfaceMod.F90 Show resolved Hide resolved

glemieux mentioned this pull request Nov 9, 2024

History cleanup task list #1282

Open

glemieux reviewed Nov 9, 2024

View reviewed changes

glemieux reviewed Nov 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and standardize per-ageclass history #1252

Refactor and standardize per-ageclass history #1252

samsrabin commented Sep 16, 2024 •

edited

Loading

samsrabin Oct 16, 2024

rgknox Oct 22, 2024

samsrabin Oct 28, 2024

samsrabin commented Oct 21, 2024 •

edited

Loading

rgknox Oct 22, 2024

samsrabin Oct 22, 2024

samsrabin Nov 11, 2024

rgknox Nov 18, 2024 •

edited

Loading

samsrabin commented Oct 29, 2024

glemieux left a comment

glemieux Nov 9, 2024

glemieux Nov 9, 2024 •

edited

Loading

glemieux left a comment

glemieux Nov 11, 2024

glemieux Nov 11, 2024

glemieux Nov 11, 2024

	! Normalize resistance diagnostics
	if (canopy_area_by_age(ipa2) .gt. nearzero) then
	hio_c_stomata_si_age(io_si,ipa2) = &
	hio_c_stomata_si_age(io_si,ipa2) / canopy_area_by_age(ipa2)

	canopy_area_by_age(cpatch%age_class) = &
	canopy_area_by_age(cpatch%age_class) + cpatch%total_canopy_area

Refactor and standardize per-ageclass history #1252

Are you sure you want to change the base?

Refactor and standardize per-ageclass history #1252

Conversation

samsrabin commented Sep 16, 2024 • edited Loading

Description:

Collaborators:

Expectation of Answer Changes:

Checklist

Documentation

Test Results:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsrabin commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgknox Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

samsrabin commented Oct 29, 2024

glemieux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemieux Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

glemieux left a 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

samsrabin commented Sep 16, 2024 •

edited

Loading

samsrabin commented Oct 21, 2024 •

edited

Loading

rgknox Nov 18, 2024 •

edited

Loading

glemieux Nov 9, 2024 •

edited

Loading