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

Grass PFT only creates leaves not cwd #891

Merged
merged 20 commits into from
Nov 16, 2022
Merged

Conversation

ZacharyRobbins
Copy link
Contributor

@ZacharyRobbins ZacharyRobbins commented Aug 16, 2022

In response to issue #884

Modified EDCohortDynamicsMod.F90 (@ln970): if the pft is grass then all of its organs are treated as leaves for calculation of coarse woody debris(cwd).
EDPatchDynamicsMod.F90 (@ln 1641): When patches spawn, only tree pft donate dead tress or cwd, and mass to the new patch, if pft is grass it assumed that grass was consumed in fire. This may not resolve fluxes to the atmosphere accurately.
EDPhysiologyMod.F90 (@ln 2199): When other litter fluxes occur, if pft is grass, then it only turnsover or contributes leaves not coarse woody debris.

Description:

Collaborators:

Expectation of Answer Changes:

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:

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

FATES baseline hash-tag:

Test Output:

ckoven and others added 3 commits August 5, 2022 15:20
In response to issue NGEET#884

Modified EDCohortDynamicsMod.F90 (@ln970): if the pft is grass then all of its organs are treated as leaves for calculation of coarse woody debris(cwd).
EDPatchDynamicsMod.F90 (@ln 1641):  When patches spawn, only tree pft donate dead tress or cwd, and mass to the new patch, if pft is grass it assumed that grass was consumed in fire. This may not resolve fluxes to the atmosphere accurately.
EDPhysiologyMod.F90 (@ln 2199):  When other litter fluxes occur, if pft is grass, then it only turnsover  or contributes leaves not coarse woody debris.
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.

There is an issue with spacing in one section. Also, why are we sending fine roots to leaves?

store_m = currentCohort%prt%GetState(store_organ, element_id)
repro_m = currentCohort%prt%GetState(repro_organ, element_id)

if ( prt_params%woody(pft) == itrue) then !trees only, May require additional accounting for the atm_fluxes when PFT is grass.
Copy link
Contributor

Choose a reason for hiding this comment

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

@ZacharyRobbins there is something going on with spacing here. Can you fix this so that the if statement is moved back to the left?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I have resolved the spacing issue, and return fine roots to their original location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this outstanding still?

Revised commit to issue on PR NGEET#891. Adjusted so fine roots are retained within that pool. Fixed the spacing on EDPatchDynamics.F90
@ckoven
Copy link
Contributor

ckoven commented Aug 24, 2022

@ZacharyRobbins Thanks so much for submitting this PR. I am trying to work through the code and have a couple questions. I'm particularly struck by the big if block in fire_litter_fluxes, which skips most of the logic for plants that aren't woody (since the termination is here). My first thought was that that shouldn't be necessary, since the currentCohort%fraction_crown_burned argument here should mean that grasses are burning up when fire occurs anyway and so shouldn't be contributing their biomass to litter when fire happens. But in staring at the (current) code, three things strike me: one is that we only let 80% of the grasses burn anyway (here), the second is that we only consider grass leaf carbon as live fuel, and not the sum of live fuel and stem carbon (here), and the third is that we then only burn off the leaf carbon and not the live stem carbon (here).

I feel like we probably need to make a choice, at least as regards to the last two points: FATES either treats all aboveground grass tissues as live fuel, and then burns then accordingly, or FATES only treats grass leaves as live fuel, and then the other aboveground tissues become litter. Does that seem right, and if so is there a strong consensus which is the better choice?

@jkshuman
Copy link
Contributor

@ckoven thanks for digging into this. The 80% limit on grass burning is a legacy piece that should be tested. Perhaps @rosiealice has insight on this, but I recall it had to do with instability? The grass pieces of the code have not changed a lot, but other aspects of the fire code have changed. For burning of grasses, I vote to include all above ground grass tissues as live fuel. If the consensus goes the other way, then I agree those tissues should go to litter.

! Absolute number of dead trees being transfered in with the donated area
num_dead_trees = (currentCohort%fire_mort*currentCohort%n * &
if ( prt_params%woody(pft) == itrue) then !trees only, May require additional accounting for the atm_fluxes when PFT is grass.
! Goal was to not add to pools in the event of grass fire. Grass would typically not introduce a flux of dead leaves
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see an else attached to this if clause where we use a logging message to indicate non-woody disturbance is not supported here, and include an endrun() and a verbose message. This prevents vulnerable code in case someone modifies disturbance generation to allow for non-woody, they will then realize it needs to be applied here as well.

@ZacharyRobbins
Copy link
Contributor Author

I think your point is correct, I think that my be a relic of testing what needed to be changed to get the grass out of the CWD pool. To clarify your point, you mean all live ag live grass tissue is treated as live fuel? or all above ground tissue (even dead tissue) as live fuel. I think it makes the most sense that all above ground grass is treated as crown consumed fuel while alive, and then all treated as leaves (or technically <1 hr fine fuels).

To clarify, some people refer to these fine fuels as live fuels (as it readily burns). In this instance I believe we are referring to live fuel as the pool called live-grass which is its own litter class (separate from dead leaves, which I think we are referring to as litter). Given that this class already exists I would say all living grass biomass should be placed in this pool, and parameters should reflect whole grass (stem, leaves) fuel characteristics. Dead grass should be treated as the pool dead leaves (as it is mostly like a <1hr fine fuel).

I can remove the if statement in fire litter flux, does that still require a logging message?
Thanks.

@jkshuman
Copy link
Contributor

jkshuman commented Sep 1, 2022

@ZacharyRobbins I agree with you on this that the live fuel for grass should include the sum of aboveground parts (only stem and leaf or also seeds?).

In the current version live grass is the only live fuel considered, with crown fire other live fuels would be considered. Once grass dies it becomes part of the dead leaves pool consisting of dead tree leaves and dead grass. So, the grass is either alive or dead. These leaf pools are separate so that the live grass can have different fuel moisture compared to dead leaves. The fuel moisture of live grass changes in response to the Nesterov Index, as all fuels do, but follows the trajectory of 1 hr fuels so it retains more moisture than the dead leaves. Fuel moisture needs focused attention, and testing grasses with hydro would also be a big improvement.

For grasses in reality they go through a process of curing as they transition from live fuels to dead fuels, and this would be an exciting enhancement to add. With curing through the year there is a gradual transition associated with die-off that directly impacts flammability with higher curing being associated with increased flammability. Considering a difference in die-off or fuel moisture for the leaves vs stem of grasses might capture these flammability changes, but that should be its own development issue IMO.

@jkshuman
Copy link
Contributor

@ZacharyRobbins thank you for moving this forward.
Can I impose on you to fix a documentation comment typo in SFMainMod with this PR?
Update "Bottom" to Wotton here:
! Information Report GLC-X-10 by Bottom et al., 2009 because there is a typo in CFFBPS Ont.Inf.Rep. ST-X-3, 1992)
The full first author name is B.M. Wotton, so I think the letters got transposed.

@ZacharyRobbins
Copy link
Contributor Author

I believe we have enacted all suggested changes. Please re-review.

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.

Looks good, but I suggest small changes to wording and organizing of comments. I also fixed a typo in the name for Wotton et al in SFMain. At a minimum that typo should be fixed.

biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Show resolved Hide resolved
biogeochem/EDPhysiologyMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPhysiologyMod.F90 Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
fire/SFMainMod.F90 Outdated Show resolved Hide resolved
fire/SFMainMod.F90 Outdated Show resolved Hide resolved
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.

I checked in with Greg on this, and since they are just edits to comments, I went ahead and committed them. The PR is approved from me.

@@ -0,0 +1,24 @@
.
Copy link
Contributor

Choose a reason for hiding this comment

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

@ZacharyRobbins @ckoven @jkshuman should this file be retained?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be deleted.

@glemieux
Copy link
Contributor

I pre-merged master into the PR branch locally on Cheyenne and ran the fates regression test suite against baseline fates-sci.1.60.0_api.24.1.0-ctsm5.1.dev112. All tests ran successfully with DIFFs aside from the expected failures. The DIFFS appear to be consistent with the changes as they start off with differences in the litter in/out and extremely small carbon balance errors and fuel variables and eventually show up in FATES_HET_RESP and FATES_NEP.

Test folder: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr891

@glemieux glemieux merged commit c7cf553 into NGEET:master Nov 16, 2022
@glemieux glemieux mentioned this pull request Nov 16, 2022
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.

5 participants