Skip to content

Conversation

@glemieux
Copy link
Contributor

@glemieux glemieux commented May 8, 2025

Description:

This pull request merges together #1358 and #1359, which are exclusively parameter file updates. This will also merge in #1355 which includes both parameter file updates and code changes associated with the parameter file update. #1355 has been tested seperately and found to be B4B.

Merging of this pull request is intended to be coordinated with ESCOMP/CTSM#3087.

Collaborators:

@ckoven @mpaiao

Expectation of Answer Changes:

Yes, the parameter value updates in #1358 and #1359 will result in answer changes to non-SP mode runs and logging runs specifically.

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:

mpaiao and others added 17 commits March 10, 2025 09:07
…ngle, multi-option

flag. This change simplifies the PFT set up and by default resolves possible incorrect
settings of 3 mutually exclusive flags. Some of the associated code was also updated from
if statements to select case, to ensure that options do not overlap.
two-stream sun-shade fraction update

This simplifies the leaf sunlit fraction calculation to assume that it
is comparible to that of other media.
this commit doesn't include the base_file refenced in the patch as that
will become available via NGEET#1358 when these are merged together
The workflow for merging these should run NGEET#1358 prior to this patch so
this patch should use the default to make sure to include NGEET#1358 changes
@glemieux glemieux added parameters: value change Pertaining to changes to the parameter file only (i.e. not removing or adding new parameters) parameters: new Pertaining to adding new parameters to the parameter file labels May 8, 2025
@glemieux glemieux moved this from Finding Reviewers to Under Review in FATES Pull Request Planning and Status May 9, 2025
@glemieux glemieux moved this from Under Review to Hold in FATES Pull Request Planning and Status May 9, 2025
@glemieux glemieux moved this from Hold to Ready to Integrate in FATES Pull Request Planning and Status May 9, 2025
@glemieux glemieux moved this from Ready to Integrate to Final Testing in FATES Pull Request Planning and Status May 9, 2025
@glemieux glemieux linked an issue May 12, 2025 that may be closed by this pull request
@glemieux
Copy link
Contributor Author

Regression testing both #1358 and #1359 against fates-sci.1.83.1_api.39.0.0-ctsm5.3.042 is complete on derecho. As expected, the satellite phenology tests show no differences, although there is a FIELDLIST diff as FATES_ZSTAR_AP is now active due to this logic in the history interface:

if ( ED_val_comp_excln .lt. 0._r8 ) then ! only valid when "strict ppa" enabled
tempstring = 'active'
else
tempstring = 'inactive'
endif
call this%set_history_var(vname='FATES_ZSTAR_AP', units='m', &
long='product of zstar and patch area by age bin (divide by FATES_PATCHAREA_AP to get mean zstar)', &
use_default=trim(tempstring), avgflag='A', vtype=site_age_r8, &

This also applies to the FatesColdAllVars test as well as that is the only other fates test that does not empty the tapes like the satellite phenology tests.

Aside from tests that engage logging or no competition, all short term tests are b4b. Per discussion with @ckoven this is expected given the changes. I.e. the logging parameter update should create a more immediate impact, while the non-logging tests should see changes that take a bit of time to propagate given the coldstart.

Results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1358-1359-fates-042

@glemieux
Copy link
Contributor Author

This PR is on hold until #1355 has been merged in and closer to when ESCOMP/CTSM#3087 is ready to go.

@glemieux glemieux force-pushed the api40-parameter-file-merge branch from b6c543c to 4d2e62c Compare May 12, 2025 21:37
Doing this to setup running the UpdateParamAPI tool from oldest to
newest pull request
@glemieux
Copy link
Contributor Author

PR #1355 remote tracking branch has been merged into this branch. I've updated the patch parameter xml files as necessary and tested them via running UpdateParamAPI.py for each of the patch files in PR number order (i.e. 1355 -> 1358 -> 1358). This is ready for ESCOMP/CTSM#3087.

@glemieux glemieux moved this from Hold to Final Testing in FATES Pull Request Planning and Status May 19, 2025
@glemieux
Copy link
Contributor Author

Final regression testing underway on derecho and izumi.

@glemieux glemieux merged commit c319e67 into NGEET:main May 20, 2025
1 check was pending
@github-project-automation github-project-automation bot moved this from Final Testing to Ready to Integrate in FATES Pull Request Planning and Status May 20, 2025
@glemieux
Copy link
Contributor Author

Regression testing is complete. Results match previous test results against fates-sci.1.84.0_api.40.0.0-ctsm5.3.042-pr1358-1359.

derecho: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1406-pr3087-fates-final
izumi: /scratch/cluster/glemieux/ctsm-tests/tests_0519-235315iz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parameters: new Pertaining to adding new parameters to the parameter file parameters: value change Pertaining to changes to the parameter file only (i.e. not removing or adding new parameters)

Projects

Development

Successfully merging this pull request may close these issues.

Need to change default value of fates_comp_excln

4 participants