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

Land Use Fixes #1273

Merged
merged 24 commits into from
Jan 31, 2025
Merged

Land Use Fixes #1273

merged 24 commits into from
Jan 31, 2025

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Oct 29, 2024

I've been running into various errors in transient historical runs with full land use. The branch I am using has various things that aren't on main yet, so I thought I'd cherry-pick just the land-use-relevant things into a separate branch, so that if others want to use them they can.

In addition to the bugfixes, this deletes a bunch of history variables that apply only to secondary lands, and replaces a subset of them (biomass, burned area, GPP, NPP) with ones that have a land-use dimension to them.

Note that there is also one ELM-side bug fix as well, at the commit pointed to in #1260.

Description:

Collaborators:

Expectation of Answer Changes:

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

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

Documentation

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 ckoven requested a review from samsrabin October 29, 2024 23:16
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.

Very nice, Charlie, thanks! I just have a few minor notes that I'll propose (untested) fixes to in a PR shortly.

biogeochem/EDLoggingMortalityMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDLoggingMortalityMod.F90 Outdated Show resolved Hide resolved
Minor changes to landuse_fixes branch
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.

Minor suggested change and two questions.

biogeochem/EDLoggingMortalityMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
Bug fix for issue NGEET#1301

Corrects when the fraction of burnt fuel is zero'd and how the litter
mass is updated based on that fraction.
@glemieux
Copy link
Contributor

glemieux commented Jan 6, 2025

Preliminary regression testing against fates-sci.1.80.6_api.37.0.0-tmp-241219.n01.ctsm5.3.016 using ckoven#26 shows nearly B4B results. Most of the DIFFS are FIELDLIST only given the history name changes. The only main DIFF is showing up in the harvest area mode LUH2 test mod, in which the FATES_PATCHAREA_LU and FATES_DISTURBANCE_RATE_MATRIX_LULU are seeing differences starting on t_index=31.

There was a run failure in the AllVars test, but this is due to the test asking from history that is removed via this PR. As such, we'll need a concurrent HLM PR to address this.

@glemieux
Copy link
Contributor

There was a run failure in the AllVars test, but this is due to the test asking from history that is removed via this PR. As such, we'll need a concurrent HLM PR to address this.

Started #1305 as a potential future update to avoid this situation.

Landuse fixes history interface deconflict with the latest tag
Add runtime checks on allometry mode settings

This adds runtime error and warning messages to avoid erroneous
`crowndepth` calculations depending on the allometry modes and the
`h2cd` allometery parameter combination.
@glemieux
Copy link
Contributor

Final regression testing on derecho is underway.

@glemieux
Copy link
Contributor

glemieux commented Jan 31, 2025

Final regression testing on derecho is passing as expected. This was tested with ESCOMP/CTSM#2934 which resulted in DIFFs for all Clm60Fates based compsets due to not being run with Meier2022. As with the preliminary testing mentioned above, there were expected DIFFs in the harvest area LUH2 test. The issue with the FatesColdAllVars test has also been resolved per ESCOMP/CTSM#2936 which was merged into ESCOMP/CTSM#2934. All other tests are b4b against fates-sci.1.80.10_api.37.0.0-ctsm5.3.021.

Results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1273-fates-ctsm53022

Izumi testing underway.

@glemieux
Copy link
Contributor

Izumi regression testing is complete. All expected tests are B4B against fates-sci.1.80.10_api.37.0.0-ctsm5.3.021 with the exception of Clm60Fates based compsets as expected.

Results: /home/glemieux/scratch/ctsm-tests/tests_0131-080811iz

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

Successfully merging this pull request may close these issues.

3 participants