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

Don't zero out patch%fuel%frac_burnt() before we use it #1302

Merged
merged 6 commits into from
Jan 3, 2025

Conversation

adrifoster
Copy link
Contributor

@adrifoster adrifoster commented Dec 11, 2024

Fixes #1301 by checking inside the TransLitterNewPatch for the type of disturbance instead of zero-ing outside of the subroutine.

Description:

Removed the lines in spawn_patches that zero-d the frac_burnt array too soon.
Added checks in TransLitterNewPatch to check for fire disturbance and a fire on the patch:
If no fire, new local variable frac_burnt set to 0.0, otherwise use patch%fuel%frac_burnt

Collaborators:

@glemieux

Expectation of Answer Changes:

Yes - should change answers because before we weren't burning any fuel

Checklist

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

Test Results:

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

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

FATES baseline hash-tag: sci.1.80.4_api.37.0.0

Test Output:

Running tests now

Copy link
Contributor

@glemieux glemieux left a comment

Choose a reason for hiding this comment

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

I like moving the check into TransLitterNewPatch. That said, do we need to make sure that currentPatch%fuel%frac_burnt is set to zero at the end of spawn_patches or is that already handled elsewhere?

biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
@adrifoster
Copy link
Contributor Author

I like moving the check into TransLitterNewPatch. That said, do we need to make sure that currentPatch%fuel%frac_burnt is set to zero at the end of spawn_patches or is that already handled elsewhere?

I added it to the top of the fire_model subroutine, where we zero other fire-related things. I don't know if this is the right place for it or not...

@glemieux
Copy link
Contributor

glemieux commented Jan 3, 2025

Regression testing on derecho underway.

@glemieux
Copy link
Contributor

glemieux commented Jan 3, 2025

Regression testing agsinst fates-sci.1.80.5_api.37.0.0-tmp-241219.n01.ctsm5.3.016 is showing DIFFs in the following tests:

FAIL ERS_Ld397.f10_f10_mg37.I2000Clm50Fates.derecho_gnu.clm-FatesCold BASELINE fates-sci.1.80.5_api.37.0.0-tmp-241219.n01.ctsm5.3.016: DIFF
FAIL ERS_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdNoComp BASELINE fates-sci.1.80.5_api.37.0.0-tmp-241219.n01.ctsm5.3.016: DIFF
FAIL ERS_Ld397.f45_f45_mg37.I2000Clm50Fates.derecho_intel.clm-FatesColdNoComp BASELINE fates-sci.1.80.5_api.37.0.0-tmp-241219.n01.ctsm5.3.016: DIFF
FAIL SMS_Lm6.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-Fates BASELINE fates-sci.1.80.5_api.37.0.0-tmp-241219.n01.ctsm5.3.016: DIFF

Spot checking the DIFFs, I think these make sense; I'm seeing changes to fire related history. @adrifoster would you take a look when you have a chance to confirm please?

Results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1302

@adrifoster
Copy link
Contributor Author

@glemieux thanks!

yep these look right to me, just to plot some differences:
here are the differences in fire_closs (the new version is higher because now we are actually burning dead fuel):

ERS_Ld397 f10_f10_mg37 I2000Clm50Fates derecho_gnu clm-FatesCold_FATES_FIRE_CLOSS_mean

which does affect ROS and fire intensity:

ERS_Ld397 f10_f10_mg37 I2000Clm50Fates derecho_gnu clm-FatesCold_FATES_ROS_mean

ERS_Ld397 f10_f10_mg37 I2000Clm50Fates derecho_gnu clm-FatesCold_FATES_FIRE_INTENSITY_mean

the new version is lower because fuel is actually lower:

ERS_Ld397 f10_f10_mg37 I2000Clm50Fates derecho_gnu clm-FatesCold_FATES_FUEL_AMOUNT_mean

@glemieux
Copy link
Contributor

glemieux commented Jan 3, 2025

Thanks @adrifoster!

@glemieux glemieux merged commit 1689466 into NGEET:main Jan 3, 2025
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Dead fuels never actually consumed in fires
2 participants