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

Refactor spitfire fuel calculations #1247

Merged
merged 65 commits into from
Nov 13, 2024

Conversation

adrifoster
Copy link
Contributor

@adrifoster adrifoster commented Sep 12, 2024

Refactors the SPITFIRE fuel calculation subroutine and adds a fuel class

Description:

Major changes to the way fuel characteristics are calculated in the SPITFIRE module. Changes are not B4B, but should be round off.

Expectation of Answer Changes:

Round off differences due to order of operation changes. Testing in progress...

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:

Testing ongoing... currently having issues with some divide by zero errors (in rate_of_spread) for only some tests... I'm not sure why this is happening since the code should check and not let SAV, bulk density, etc. equal zero.

/glade/derecho/scratch/afoster/tests_0912-094039de

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

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

FATES baseline hash-tag:

Test Output:

@rgknox rgknox self-requested a review October 18, 2024 15:16
@adrifoster
Copy link
Contributor Author

@rgknox asked for some functional testing plots:

Plot of Nesterov Index over time using driver data from an interior Alaska site

Nesterov_plot

Corresponding fuel moisture for above NI values, for 3 different synthetic fuel models (see module

fuel_moisture_plot

Fuel loading for fuel types:

Fuel loading_plot

Fractional loading

Fractional fuel loading_plot

Fuel bulk density

Fuel bulk density_plot

Fuel SAV

Fuel surface area to volume ratio_plot

fire/FatesFuelMod.F90 Outdated Show resolved Hide resolved
@rgknox rgknox requested a review from samsrabin October 18, 2024 16:18
@rgknox
Copy link
Contributor

rgknox commented Oct 18, 2024

One general comment. This is something that was already in the code that kind of bothered me. We all know we break up litter into component groups for both decomposition and fire. Both modules use almost the same groups, which are governed by size and location (above and below ground). In general, I'd like this to be a little cleaner, where the super-set of all delineations is contained in a process agnostic module, and decomposition and fire could inherit those delineations and group/use them how they see fit. I don't see this as in-scope for this PR necessarily, but I'm curious how you see all this, and if its still a problem.

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.

Definite improvement, @adrifoster has already addressed my comments. Thanks!

@adrifoster
Copy link
Contributor Author

One general comment. This is something that was already in the code that kind of bothered me. We all know we break up litter into component groups for both decomposition and fire. Both modules use almost the same groups, which are governed by size and location (above and below ground). In general, I'd like this to be a little cleaner, where the super-set of all delineations is contained in a process agnostic module, and decomposition and fire could inherit those delineations and group/use them how they see fit. I don't see this as in-scope for this PR necessarily, but I'm curious how you see all this, and if its still a problem.

Totally agree. I wanted to try to merge them somehow but they are slightly different in terms of how they are grouped and used, so I wasn't sure how to do it efficiently for this PR. I think it warrants a small group of us sitting down to discuss. If we merged them it would also make further discretizing the litter/fuel easier (i.e. PFT-specific, etc.).

I also don't love that the litter is decayed in one spot, the fuel is combusted in another, etc. And seemingly we do (?) make sure these things align?

Copy link
Contributor

@samsrabin samsrabin left a comment

Choose a reason for hiding this comment

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

Great work! This is so much better organized and easier to understand. I just have some rather minor comments.

fire/FatesFuelClassesMod.F90 Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
biogeochem/FatesPatchMod.F90 Show resolved Hide resolved
fire/FatesFuelClassesMod.F90 Outdated Show resolved Hide resolved
fire/FatesFuelMod.F90 Outdated Show resolved Hide resolved
testing/functional_testing/fire/fuel_plotting.py Outdated Show resolved Hide resolved
testing/testing_shr/FatesArgumentUtils.F90 Outdated Show resolved Hide resolved
testing/testing_shr/FatesArgumentUtils.F90 Outdated Show resolved Hide resolved
testing/unit_testing/fire_fuel_test/CMakeLists.txt Outdated Show resolved Hide resolved
testing/unit_testing/fire_fuel_test/test_FireFuel.pf Outdated Show resolved Hide resolved
@adrifoster
Copy link
Contributor Author

Great work! This is so much better organized and easier to understand. I just have some rather minor comments.

Thanks @samsrabin! Great suggestions, will get started on applying them.

@adrifoster
Copy link
Contributor Author

I've addressed @samsrabin's comments and made all suggested changes.

@adrifoster
Copy link
Contributor Author

Thanks to @glemieux for finding the issue with this PR. All tests now pass.

@adrifoster adrifoster merged commit e3e7d2c into NGEET:main Nov 13, 2024
1 check passed
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.

4 participants