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

Overhaul of FATES history variable outputs #802

Merged
merged 51 commits into from
Nov 29, 2021

Conversation

adrifoster
Copy link
Contributor

@adrifoster adrifoster commented Nov 2, 2021

Standardized FATES history variable names, units, and changed the flushing logic.

Description:

This is a set of changes intended to make the history output more clear to users (new and otherwise), with standardized units and unit descriptions in the metadata, standardized naming conventions for how the variables are indexed, and more verbose long names. This also updates how we treat non-FATES columns in the upscaling to the grid-cell level output. This resolves #630 and #535

This PR has an associated ctsm pull request: ESCOMP/CTSM#1542

  1. Almost all output variable names have changed. All FATES variables start with a "FATES_" prefix, and also have a suffix that denotes if/how it has been indexed:

Suffixes:
_AC: cohort/plant age
_AP: patch/disturbance age
_CL: canopy layer
_DC: coarse woody debris size
_LL: leaf layer
_EL: element
_FC: fuel class
_HT: height
_PF: PFT
_SL: soil layer
_SZ: size

Variables can be indexed by multiple variables (e.g. _SZPF). Variables that don't have one of these suffixes are only site-level. See the attached csv file to see a table of the old name and new name for each variable (note that some have been deleted - see the notes column)

  1. Units have also been changed.

As per a recent (09/16/21) FATES call, we decided to standardize all units such that most align with the units in the CF table. The description of the units in the metadata has also been standardized as per CF conventions (e.g. 'kg m-2 s-1').

Note that we no longer denote carbon (or another element) in the units for biomass (or other similar) variables, but the long name has been updated to make it clear that the mass is in kg of carbon. Note that for per area cohort/population density the units description is "m-2".

Major Units Changes

  • mass is in kg
  • time is in seconds for "fast" (i.e. photosynthesis) variables and years for "slow" (i.e. population/demographics)
  • length is in meters
  • area is in meters squared

See the attached csv to see the old units, the new units, and what the factor difference should be between them. Also see Charlie's notes from the meeting here.

  1. General clean-up - some typos/inaccuracies in the metadata and mistakes in the code were fixed.

  2. Flushing logic. We now flush all variables on the FATES side to zero, whereas all variables on the host land model side are flushed to the ignore value.

FATES_historyvar_changes.csv

Collaborators:

Discussions with @rgknox , @ckoven, @glemieux, @rosiealice, @jkshuman, @billsacks

Expectation of Answer Changes:

These should not change the answers per se, but will result in factor differences between this update and master because of the units changes. See the attached csv for the expected factor differences. I have done testing to determine that where we do get differences, they are the expected differences based on the units change.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
  • I have read the CONTRIBUTING document.
  • FATES PASS/FAIL regression tests were run
  • If answers were expected to change, evaluation was performed and provided

Test Results:

CTSM (or) E3SM (specify which) test hash-tag: ctsm5.1.dev061-10-gddaa7017

CTSM (or) E3SM (specify which) baseline hash-tag: ctsm5.1.dev061

FATES baseline hash-tag: sci.1.49.0_api.17.0.0-48-g8777a62a

Test Output:

Almost all failed tests on Izumi (aux_clm) and Cheyenne (aux_clm and fates) are only for NLCOMP (namelist changes - which we expected) and FIELDLIST diffs (which we also expected since the FATES history variables changed names).

There was a DIFF FAIL for SMS_Lm3_D_Mmpi-serial.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdDefHydro in aux_clm on Cheyenne and ERS_D_Ld5.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdDefHydro and SMS_Lm3_D_Mmpi-serial.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdDefHydro in fates, but this was because of diffs for FATES_SOILMATPOT_SL and FATES_ROOTUPTAKE_SL , which changed units (variable names did not change), so this was expected.

DIFF Fail on Izumi for FAIL SMS_D_Vnuopc_Mmpi-serial.CLM_USRDAT.I1PtClm51Bgc.izumi_nag.clm-default--clm-NEON-NIWO, I discussed this with @billsacks, @ekluzek, and @wwieder and it seems to be a problem with the baseline itself and the fact that the NEON datasets sometimes change.

In general, I also tested output to ensure that when answers did change, they changed in the correct way (i.e. by the expected factor difference). Some variables changed because we fixed mistakes in the code:

site_hydr%sapflow_scpf was getting set to hio_rootuptake_si, which also overwrote the hio_rootuptake_si. This was updated so that sapflow was set to hio_sapflow_si

@JoshuaRady found a bug in that this%hvars(ih_fines_bg_elem)%r82d and this%hvars(ih_fines_ag_elem)%r82d were switched in the associate statement. This has also been fixed.

A few other differences are seemingly from the flushval changes:

  • NPP_LEAF_CANOPY_SCLS/FATES_LEAF_ALLOC_CANOPY_SZ - in master output this value is -1000 for much of the first few time steps
  • CEFFLUX/FATES_CEFFLUX - off by a factor of ~0.98, unclear where this exactly comes from
  • CEFFLUX_SCPF/FATES_CEFFLUX_SZPF - also off but ~0.99
  • SITE_MEANLIQVOL_DROUGHTPHEN/FATES_MEANLIQVOL_DROUGHTPHEN - off by ~0.996, seemingly from flushval update

FactorDifference

@glemieux glemieux linked an issue Nov 2, 2021 that may be closed by this pull request
@glemieux glemieux added the ctsm An issue is related to ctsm host land model or a particular PR has a corresponding ctsm-side PR label Nov 2, 2021
@adrifoster adrifoster changed the title Overhaul of FATES history variable outputs #630 Overhaul of FATES history variable outputs Nov 3, 2021
@adrifoster
Copy link
Contributor Author

I will note that there is a potential issue that may or may not need to get solved. Currently, we need to set the individual hio_m<x>_scpf(io_si, :) to 0.0_r8 at the top of the site loop in update_history_dyn in FatesHistoryInterfaceMod.90 (see lines here).

This is because here we sum up all of these values using the actual hio_m<x>_scpf variables, rather than the more internal patch%cohort instances. Because of the new way our flushing works, on the very first time the update_history_dyn subroutine is called (at least in CTSM) during a restart or init_coldstart (see here), we haven't yet flushed these dynamics variables to 0.0_r8, which we currently do here in EdMainMod.90 in ed_ecosystem_dynamics. Thus, when they are summed on this first time step you get a massively large number. Setting the individual hio mortality values to 0.0 at the top of the site loop fixes this issue.

@adrifoster
Copy link
Contributor Author

Also note that these updates go along with an update to CTSM where we now call flush_hvars inside the init_coldstart and restart subroutines in clmfates_interfaceMod.F90 (here and here).

This branch is here.

@glemieux glemieux linked an issue Nov 8, 2021 that may be closed by this pull request
@glemieux glemieux requested a review from rgknox November 8, 2021 21:14
main/EDMainMod.F90 Outdated Show resolved Hide resolved

model_day_int = nint(hlm_model_day)
siteloop: do s = 1,nsites
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

ivar=0

Copy link
Contributor

Choose a reason for hiding this comment

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

@adrifoster , could we add in the suffix key at the top here, so future authors have a quick reference on how to name output variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, great idea. I will do that!

Copy link
Contributor

Choose a reason for hiding this comment

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

looks good

Copy link
Contributor

@rgknox rgknox left a comment

Choose a reason for hiding this comment

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

This looks great. Only one suggestion, that is to add in the suffix key at the top of the output definition section so future authors have a guide to naming new variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ctsm An issue is related to ctsm host land model or a particular PR has a corresponding ctsm-side PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History Refactor Wish-list change history flushing behavior
3 participants