Skip to content

Conversation

@mpaiao
Copy link
Contributor

@mpaiao mpaiao commented Apr 14, 2025

Description:

This is the first step towards refactoring the phenology code. As described in #1385, in this step I create a new module that manages the cumulative/memory variables (main/FatesCumulativeMemoryMod.F90), with a main (public) sub-routine that handles all the updates (UpdateCumulativeMemoryVars) and calls sub-routines that update the date-, temperature- and moisture-related memory variables.

As described in #1385, a new module was necessary (instead of adding to the existing FatesRunningMeanMod) to avoid module circularity, as the new module uses EDTypesMod and EDTypesMod.F90 uses FatesRunningMeanMod.

Collaborators:

@XiulinGao @ckoven @rgknox @lmkueppers @rosiealice @glemieux

Expectation of Answer Changes:

At this point, the results should be bit-for-bit.

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

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

Integrator

  • FATES PASS/FAIL regression tests were run
  • Evaluation of test results for answer changes was performed and results provided
  • FATES-CLM6 Code Freeze: satellite phenology regression tests are b4b

If satellite phenology regressions are not b4b, please hold merge and notify the FATES development team.

Documentation

Test Results:

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

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

FATES baseline hash-tag:

Test Output:

@mpaiao
Copy link
Contributor Author

mpaiao commented Apr 21, 2025

Probably integrate #1355 before this one.

samsrabin and others added 14 commits May 10, 2025 17:13
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
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
(Also add postprocessing info, where needed.)
- FATES_NPLANT_SZAP
- FATES_NPLANT_CANOPY_SZAP
- FATES_NPLANT_USTORY_SZAP
- FATES_NPLANT_SZAPPF
- FATES_NPATCH_AP
- FATES_CANOPYAREA_AP
- FATES_SECONDAREA_ANTHRODIST_AP
- FATES_SECONDAREA_DIST_AP
- FATES_PATCHAREA_AP
- Unused fire rate-of-spread output
samsrabin and others added 21 commits May 10, 2025 17:23
Useful for checking that FATES_PRIMARY_AREA_AP has the correct denominator.
…_AP.

For consistency with other ageclass-stratified outputs.
… counts, and updating everything else that dependons on that.
@glemieux glemieux added science: phenology type: refactor Restructures code without changing functionality labels May 19, 2025
! over the short timestep. We turn off these variables when we want
! to save time (and some space)

call this%set_history_var(vname='FATES_NPP_AP', units='kg m-2 s-1', &
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This registry is a duplicate. As far as I can tell, we already have a registry that is at the dynamics time-scale and the complex dimension scale. That registry is consistent with how the diagnostic is updated, whereas this diagnostics is expecting a variable that is updated at the high frequency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this, @rgknox, I can remove it.



! Update elapsed time
call UpdatePhenologyDate(currentSite)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verifying that order of operations is preserved. This checks, currentSite%phen_model_date was/is incremented before its use in phenology(), and this is still true.

! Call a routine that will compute cumulative variables and "memory" averages for
! a suite of variables. These variables are mostly used for leaf phenology, but they
! may be useful for other components (disturbances, mortality, management).
call UpdateCumulativeMemoryVars(currentSite,bc_in)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checked that all memory variables that were refactored here were being incremented before they were being used in their original use, and this is true.

! restart file)
currentSite%phen_model_date = currentSite%phen_model_date + 1
! This is the elapsed number of days since the very beginning of the simulation time
model_day_int = currentSite%phen_model_date
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this local variable (model_day_int) decreases the readability of the code and we should ditch it. @mpaiao I might make a PR to your branch with this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, go for it, thanks! Indeed I wasn't sure why we needed model_day_int.

temp_in_C = CurrentSite%oldest_patch%tveg24%GetMean() - tfrz
end if
! Retrieve average canopy temperature of the current day
temp_in_C = currentSite%vegtemp_memory(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment that index 1 is the present day and was updated at the beginning of the dynamics step. We should probably change the language to reflect that index 1 is the previous 24 hours. (its not actually the present day, as the dynamics happens on the first time-step of the day).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

science: phenology type: refactor Restructures code without changing functionality

Projects

Status: Under Review

Development

Successfully merging this pull request may close these issues.

5 participants