NGEET / fates

repository for the Functionally Assembled Terrestrial Ecosystem Simulator (FATES)
Other
105 stars 92 forks source link

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

Closed samsrabin closed 2 months ago

samsrabin commented 4 months ago

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

Integrator

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:

samsrabin commented 4 months ago

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 commented 4 months ago

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 1ec6d6eb.

❌ → ✅ 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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

glemieux commented 3 months ago

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

samsrabin commented 2 months ago

Superseded by PR #1252.