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

FATES API Updates to facilitate refactor #2000

Merged
merged 36 commits into from
Aug 10, 2023

Conversation

adrifoster
Copy link
Collaborator

Description of changes

updates, mostly to module uses, to facilitate a FATES PR that involves a heavy refactor of the patch and cohort types.

Specific notes

All changes should be b4b. I also updated to the latest ccsconfig branch to bring in an MPI bug fix.

CTSM Issues Fixed (include github issue #):

Are answers expected to change (and if so in what way)?

No answer changes, everything is B4B

Any User Interface Changes (namelist or namelist defaults changes)?

No

Testing performed, if any:
fates test suite on Cheyenne - all B4B

Will test aux_clm and then test on izumi as well to ensure everything is B4B

Copy link
Collaborator

@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.

There is one minor typo in a comment.

src/utils/clmfates_paraminterfaceMod.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

I just have a few comments and questions. Obviously most of the work for this is inside FATES. The FATES version does need to be updated in Externals_CLM.cfg.

There isn't much at this level, and it all looks good. I'm happy for the refactoring going on. The principles of the changes make sense to me.

Externals.cfg Show resolved Hide resolved
Externals.cfg Show resolved Hide resolved
src/utils/clmfates_paraminterfaceMod.F90 Outdated Show resolved Hide resolved
src/utils/clmfates_paraminterfaceMod.F90 Show resolved Hide resolved
src/utils/clmfates_paraminterfaceMod.F90 Outdated Show resolved Hide resolved
Add parameterization to allow excess ice in soil and subsidence

Description:
Parameterization for excess ice described in Lee et al. (2014):
http://dx.doi.org/10.1088/1748-9326/9/12/124006

This code is a modified version of code provided by Lei Cai:
https://github.com/lca041/ctsm/tree/clm5.0.dev92_exice

Works only for the nuopc driver.
@glemieux
Copy link
Collaborator

glemieux commented Aug 8, 2023

Regression testing with aux_clm on izumi and cheyenne are underway.

@glemieux
Copy link
Collaborator

glemieux commented Aug 8, 2023

aux_clm regression testing on izumi is complete. All non-fates tests are b4b as expected. The fates tests result in DIFFs against ctsm5.1.dev132 as we are updating the fates pointer from sci.1.66.1_api.25.5.0 to sci.1.67.1_api.26.0.0, which includes intermediate non-b4b science updates. The fates suite tests against the last fates tag are indeed b4b as can be seen in NGEET/fates#1024 (comment).

Still waiting on aux_clm testing on cheyenne to complete.

Izumi results location: /home/glemieux/scratch/ctsm-tests/tests_pr1024-aux_clm

@glemieux
Copy link
Collaborator

glemieux commented Aug 8, 2023

I'm seeing all the nvhpc tests fail on cheyenne (including the expected failure) during the aux_clm regression testing: Otherwise all other expected tests are b4b:

    FAIL SMS_D.f10_f10_mg37.I2000Clm51BgcCrop.cheyenne_nvhpc.clm-crop RUN time=15 (EXPECTED FAILURE)
    FAIL SMS.f10_f10_mg37.I2000Clm50BgcCrop.cheyenne_nvhpc.clm-crop RUN time=16
    FAIL SMS.f45_f45_mg37.I2000Clm51FatesSpRsGs.cheyenne_nvhpc.clm-FatesColdSatPhen RUN time=11

The cesm logs for the unexpected failures appear to be failing in a manner similar to the expected failure.

@glemieux
Copy link
Collaborator

glemieux commented Aug 9, 2023

It looks like the other nvhpc tests are failing due to the externals update that incremented the ccs_config_cesm version. Retesting one of the nvhpc testmods from ctsm5.1dev132 resulted in the test passing. Upon seeing this, I manually reverted the ccs_config_cesm change on the local version of this pr branch and retested, which resulted in the test passing. Results can be found here:
/glade/u/home/glemieux/scratch/ctsm-tests/tests_pr2000-ccs_config_cesm_revert

@ekluzek given this, should I push the commit to revert the change and retest or should I mark these as expected failures? Regardless, I'm assuming I should make an issue, correct?

@ekluzek
Copy link
Collaborator

ekluzek commented Aug 9, 2023

@glemieux leave the ccs_config version as it is, and file an issue. I think this will be resolved with an update of the externals. We do want to update externals to cesm2_3_beta15. I'm thinking that is likely to fix that. We updated the ccs_config because there was a slow down that we think this version resolves. So ust adding two expected fails isn't that bad...

@glemieux
Copy link
Collaborator

glemieux commented Aug 9, 2023

Roger that @ekluzek. Given that the failure mode looks very similar to #1733, I'll update that issue and reference it in the expected failures list. Once that is set, this PR should be good to go.

@ekluzek ekluzek self-assigned this Aug 10, 2023
@ekluzek ekluzek added code health improving internal code structure to make easier to maintain (sustainability) FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM labels Aug 10, 2023
@ekluzek ekluzek merged commit 942de5c into ESCOMP:master Aug 10, 2023
2 checks passed
@ekluzek ekluzek deleted the fates_refactor branch August 10, 2023 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health improving internal code structure to make easier to maintain (sustainability) FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM
Projects
Status: Done (non release/external)
Development

Successfully merging this pull request may close these issues.

3 participants