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

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

Closed
wants to merge 21 commits into from

Conversation

samsrabin
Copy link
Contributor

Description:

Will resolve #1205.

Collaborators:

@glemieux @rgknox @ckoven

Expectation of Answer Changes:

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

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:

FATES_PATCHAREA_LU and FATES_PATCHAREA_AP are not in units of m2, but rather m2/m2. It is thus misleading for them to be named "AREA". In the interest of not messing up user workflows, I'm not changing those---at least for now. However, this commit renames related internal variables in the history code for clarity.

Specifically, "area" is replaced with "fracarea" (fractional area, relative to site area) in the following:
- ih_area_si_landuse
- ih_area_si_age
- hio_area_si_landuse
- hio_fracarea_si_age
1) Makes it clearer that this is a patch-level variable.
2) Uses "fracarea" instead of "area" for clarity and consistency with previous commit.
- FATES_ZSTAR_AP
- FATES_SECONDAREA_ANTHRODIST_AP
- FATES_SECONDAREA_DIST_AP
- FATES_BURNFRAC_AP
- FATES_FIRE_INTENSITY_BURNFRAC_AP
- FATES_VEGC_AP
- FATES_FUEL_AMOUNT_AP
- FATES_CANOPYAREA_AP
@samsrabin samsrabin self-assigned this Jul 17, 2024
@samsrabin samsrabin marked this pull request as draft July 17, 2024 19:16
Using fates_patch_type%area_by_age allows consolidation into one block, rather than splitting the calculation across two parts of update_history_hifrq2.
Moving calculation of canopy_area_by_age to an earlier patch loop allows consolidation into one block, rather than splitting the calculation across two parts of update_history_hifrq2.

! Increment the fractional area in each age class bin
hio_area_si_age(io_si,cpatch%age_class) = hio_area_si_age(io_si,cpatch%age_class) &
hio_fracarea_si_age(io_si,cpatch%age_class) = hio_fracarea_si_age(io_si,cpatch%age_class) &
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and the equivalent hio_fracarea_si_landuse code below can now actually be moved out of the patch loop and into an age-class loop.

@samsrabin
Copy link
Contributor Author

samsrabin commented Jul 18, 2024

As of 8e7a1d85, diffs are as expected: only in _AP variables (plus FATES_SCORCH_HEIGHT_APPF), and root mean square differences all < 1e-15.

@samsrabin
Copy link
Contributor Author

samsrabin commented Jul 23, 2024

Unfortunately, my test script indicates that most variables are still not getting the right answer. Here, ✅ indicates that ageclass-area-weighted mean of the per-ageclass (_AP) variable matches the non-per-ageclass variable, and ❌ indicates otherwise. (Comparisons use the numpy isclose() method with default options.) The first symbol indicates the result from before I started making changes, and the second is from commit 1ec6d6e.

❌ → ✅ FATES_BURNFRAC(_AP):
     max abs diff = 5.9e-08 → 8.99e-10
     max rel diff = 99.9% → 52.5%
❌ → ❌ FATES_DDBH_CANOPY_SZ(AP):
     max abs diff = 0.0262 → 0.000369
     max rel diff = 99.1% → 13.0%
❌ → ❌ FATES_DDBH_USTORY_SZ(AP):
     max abs diff = 0.0221 → 0.000118
     max rel diff = 96.2% → 2.3%
❌ → ❌ FATES_FIRE_INTENSITY_BURNFRAC(_AP):
     max abs diff = 2.62e+04 → 323
     max rel diff = 99.9% → 52.5%
❌ → ❌ FATES_FUEL_AMOUNT(_AP):
     max abs diff = 0.506 → 0.0108
     max rel diff = 59.3% → 5.5%
❌ → ❌ FATES_FUEL_AMOUNT_(AP)FC:
     max abs diff = 0.354 → 0.00708
     max rel diff = 96.1% → 7.9%
✅ → ✅ FATES_GPP(_AP)
❌ → ❌ FATES_LAI(_AP):
     max abs diff = 0.00524
     max rel diff = 7.7%
❌ → ❌ FATES_LBLAYER_COND(_AP):
     max abs diff = 0.0962
     max rel diff = 8.5%
❌ → ❌ FATES_MORTALITY_CANOPY_SZ(AP):
     max abs diff = 17.5 → 17.5
     max rel diff = 99.1% → 116.7%
❌ → ❌ FATES_MORTALITY_USTORY_SZ(AP):
     max abs diff = 2.63e+03 → 2.63e+03
     max rel diff = 100.0% → 100.0%
❌ → ❌ FATES_NPLANT_CANOPY_SZ(AP):
     max abs diff = 30.4 → 2.15
     max rel diff = 99.1% → 7.3%
❌ → ❌ FATES_NPLANT_SZ(AP):
     max abs diff = 67.3 → 2.15
     max rel diff = 99.1% → 7.3%
❌ → ❌ FATES_NPLANT_USTORY_SZ(AP):
     max abs diff = 58.3 → 0.996
     max rel diff = 96.2% → 3.8%
✅ → ✅ FATES_NPP(_AP)
❌ → ✅ FATES_NPP_(AP)PF:
     max abs diff = 6.06e-08 → 2.18e-10
     max rel diff = 247.0% → 65.9%
❌ → ❌ FATES_STOMATAL_COND(_AP):
     max abs diff = 0.00179
     max rel diff = 41.4%
❌ → ❌ FATES_VEGC(_AP):
     max abs diff = 0.446 → 0.0118
     max rel diff = 58.6% → 7.4%
❌ → ❌ FATES_VEGC_(AP)PF:
     max abs diff = 0.312 → 0.00849
     max rel diff = 100.0% → 100.0%

🤷 Non-per-age equivalent not in Dataset:
     FATES_CANOPYAREA(_AP)
     FATES_NCL(_AP)
     FATES_NPATCH(_AP)
     FATES_PATCHAREA(_AP)
     FATES_SCORCH_HEIGHT_(AP)PF
     FATES_SECONDAREA_ANTHRODIST(_AP)
     FATES_SECONDAREA_DIST(_AP)
     FATES_ZSTAR(_AP)

🤷 Too many (> 2) duplexed dimensions:
     FATES_NPLANT_SZ(AP)PF

This is from a 4-year, 10x15 run with CTSM on Derecho.

@samsrabin
Copy link
Contributor Author

samsrabin commented Jul 24, 2024

Looks like isclose() might just be too strict. Plotting the discrepancies as box plots, it looks like most are much better. I think this PR would thus be an improvement, even if it doesn't end up being perfect.

I'll have a look at the unchanged variables before marking this PR as ready for review.

@samsrabin
Copy link
Contributor Author

samsrabin commented Jul 24, 2024

A note: I've updated my analysis script to use FATES_CANOPYAREA_AP as the weighting for FATES_STOMATAL_COND_AP and FATES_LBLAYER_COND_AP. However, because FATES_CANOPYAREA_AP has also been fixed as part of this PR, it can't be used straight-up as a weight. Instead, as of 5942a0d, it must be multiplied by FATES_PATCHAREA_AP. This can result in a slightly worse discrepancy between the _AP and non-_AP variables, since there's more opportunity for imprecision to slip in.

@samsrabin
Copy link
Contributor Author

Alright, most everything is improved now; see the latest analysis outputs comparing before I did anything vs. f21fa95.

Incremental changes:

I think this is ready to have someone look at it. Any suggestions as to what I'm doing wrong with the mortality variables would be much appreciated, but it'd be good to at least get opinions on the other stuff.

@samsrabin samsrabin marked this pull request as ready for review July 25, 2024 20:05
@samsrabin samsrabin added history output Pertaining to FATES history output variables and removed draft labels Jul 25, 2024
@samsrabin samsrabin changed the title [WIP] Refactor and fix per-age-class history outputs Refactor and fix per-age-class history outputs Aug 5, 2024
@rgknox rgknox requested review from ckoven and rgknox August 5, 2024 19:11
@glemieux
Copy link
Contributor

@rgknox @ckoven and @samsrabin to schedule a meeting to review.

@samsrabin samsrabin marked this pull request as draft August 22, 2024 18:56
@samsrabin
Copy link
Contributor Author

Superseded by PR #1252.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug - software engineering history output Pertaining to FATES history output variables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect (?) calculation of per-age-class outputs
2 participants