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

Bug fix for issue #1289 #1291

Merged
merged 4 commits into from
Dec 9, 2024
Merged

Bug fix for issue #1289 #1291

merged 4 commits into from
Dec 9, 2024

Conversation

sshu88
Copy link
Contributor

@sshu88 sshu88 commented Nov 20, 2024

Fix the issue of no wood product when forced by area-based harvest. #1289

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:

Fix the issue of no wood product when forced by area-based harvest. 
NGEET#1289
@rgknox rgknox requested a review from XiulinGao December 2, 2024 15:22
Copy link
Contributor

@XiulinGao XiulinGao left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@@ -291,7 +291,7 @@ subroutine LoggingMortality_frac( pft_i, dbh, canopy_layer, lmort_direct, &
! transfer of area to secondary land is based on overall area affected, not just logged crown area
! l_degrad accounts for the affected area between logged crowns
if(prt_params%woody(pft_i) == itrue)then ! only set logging rates for trees
if (cur_harvest_tag == 0) then
if (cur_harvest_tag == 0 .or. cur_harvest_tag == 2) then
Copy link
Contributor

Choose a reason for hiding this comment

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

HI @sshu88 -- would it be possible to replace these integers that you are comparing cur_harvest_tag to, here and elsewhere, with named integer constants that describe more clearly what these conditions refer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion! I updated the code.

Use named integer constants for "harvest_tag"
Define named constants for "harvest_tag"
@glemieux
Copy link
Contributor

glemieux commented Dec 2, 2024

Regression testing underway on derecho.

Two-stream singularity correction update

This implements a more comprehensive avoidance of singularities with
two-stream radiation across bands.
@glemieux glemieux linked an issue Dec 3, 2024 that may be closed by this pull request
@glemieux
Copy link
Contributor

glemieux commented Dec 9, 2024

Regression testing is showing b4b results for all expected tests. Note that the relevant test for this fix FatesColdLandUse is currently broken due to ESCOMP/CTSM#2810 (which will be fixed in an upcoming pull request). For now I manually modified the test to directly point to the necessary landuse timeseries file and the test passed.

Results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1289-fates
Manual land use test: tests_pr1291-ctsm3014-direct

@glemieux
Copy link
Contributor

glemieux commented Dec 9, 2024

Testing the fates_cold_landuse testmod via e3sm on perlmutter exposed unrelated issue E3SM-Project/E3SM#6806. Fixing this via E3SM-Project/E3SM@f7267be results in the e3sm landuse test passing. Previously this test was passing, but not actually engaging the landuse_timeseries test (for e3sm only).

Results: /global/homes/g/glemieux/scratch/e3sm-tests/pr1291-landuse-diag3-6806fix.fates.pm-cpu.gnu.Ca714d58227-F77e89489

@glemieux glemieux merged commit 296e1d6 into NGEET:main Dec 9, 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
Status: Ready to Integrate
Development

Successfully merging this pull request may close these issues.

Logging direct mort condition check bug
4 participants