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

Updates to fire history variables #640

Merged
merged 9 commits into from
May 11, 2020
Merged

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Apr 28, 2020

A bunch of updates to fire-related history variables, including moving all of the ones that had been on the patch structure off of it, fixing the fire carbon loss variable to not be zero'd out before history writing, and adding some further diagnostics.

Description:

Sort of a grab bag of changes, but all related to outputting fire behavior information to the history fields:

  1. Eleven variables that had been on the patch structure needed to get moved off of it (so that we can deprecate that way of doing history weighting, see History Refactor Wish-list #630).
  2. the variable Fire_Closs was just passing zeros, because the structure it is on was getting zeroed before the history call. I also added another variable, FIRE_FLUX , which has the PARTEH ELEMENT dimension, so that when we start adding nutrients we can keep track of fire-related N and P losses too. Its possible that we could thus delete Fire_Closs as redundant to the carbon index of FIRE_FLUX but I haven't done that here.
  3. some of the variables, such as fire intensity and rate of spread, were being averaged in an evenly-weighted manner, whether or not a fire had occurred on a given patch on a given timestep. Since we typically look at monthly or longer period for time averaging, these were thus getting diluted by the many times when no fire was occurring. So I added a few new variables, like the product (intensity * area). That way one can calculate the longer-term burned-area-weighted mean intensity as the ratio mean(intensity * area) / mean(area). The variables that I've added this for are intensity, rate of spread, and total fuel consumption. The names on this are a bit awkward, but so far I am thinking that calling them XXX_AREA_PRODUCT is simplest so that one can see that they only make sense if you divide by burned area; so they are currently: FIRE_INTENSITY_AREA_PRODUCT, FIRE_ROS_AREA_PRODUCT, and FIRE_TFC_ROS_AREA_PRODUCT. I don't love that, but it seemed safer than calling them averages, since they are not actually average quantities, but needed to calculate the averages.
  4. I added three new patch-age-structured variables and one new fuel-pool-structured variable so as to be able to better see what's going on in the fire model. The three patch-age-structured variables are: AREA_BURNT_BY_PATCH_AGE, FIRE_INTENSITY_BY_PATCH_AGE, and SUM_FUEL_BY_PATCH_AGE. The fuel-pool-structured variable is the fraction of fuel combusted, which I output as in (3) above, as multiplied by burned area so that one can calculate burned-area-weighted means of this.

Collaborators:

changes based on discussions with @jkshuman, @rgknox

Expectation of Answer Changes:

Expected changes only to the history variables that I've messed with in some way. I'm not totally sure that I understand why changing the _pa variables to use the site-level weighting leads to greater than roundoff changes, but I think maybe it has to do with patches where the canopy isn't closed not having been weighted properly in the old system (which is part of the reason why we want to get rid of that).

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:

none as yet.

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

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

FATES baseline hash-tag:

Test Output:

@glemieux glemieux requested a review from jkshuman April 30, 2020 16:28
@glemieux glemieux self-assigned this Apr 30, 2020
@glemieux
Copy link
Contributor

glemieux commented May 6, 2020

This looks good to me. Regression testing underway.

Copy link
Contributor

@jkshuman jkshuman left a comment

Choose a reason for hiding this comment

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

@ckoven @glemieux a few minor edits, but looks good.

main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
@rgknox
Copy link
Contributor

rgknox commented May 6, 2020

I like this PR, good stuff.

One thing to note. When you use flushval = 0.0, like here:

https://github.com/NGEET/fates/pull/640/files#diff-682b3e29ce096fa74f8fa95ff48735c4R4122

You are zero'ing literally every column, and will potentially include some zeros from non-fates columns. In the next refactor of the history variables, we will automatically flush non-fates columns to the ignore value. If you want to be assured that you are not including zero's from non-fates columns in your average, flush to the ignore value, and make sure to pre-zero any column level diagnostics that are tallied up over patches and cohorts.

@glemieux
Copy link
Contributor

glemieux commented May 6, 2020

All expected PASS:

/glade/u/home/glemieux/scratch/clmed-tests/pr640-fire-hist-vars-C242d6d14-F82cb0e45.fates.cheyenne.intel
/glade/u/home/glemieux/scratch/clmed-tests/pr640-fire-hist-vars-C242d6d14-F82cb0e45.fates.cheyenne.gnu

Confirmed all DIFFs due to changed fire variables.

Test didn't include most recent commit, but I think we can ignore that given the changes were to vname or long only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants