Skip to content

Fix FATES electron transport variable being set prior to passing to FATES #3063

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 9, 2025
Merged

Fix FATES electron transport variable being set prior to passing to FATES #3063

merged 6 commits into from
May 9, 2025

Conversation

glemieux
Copy link
Collaborator

@glemieux glemieux commented Apr 10, 2025 •
edited
Loading

Description of changes

This fixes #3062.

It also fixes an occurrence of a possible silent bug (see #3066) in which the fates_electron_transport_model was not being included into the lnd_in file due to the namelist definition group value being incorrectly spelled.

This also adds a new testmod to exercise the non-default option for the FATES electron transport namelist option. currently runs, but we result in a diff once NGEET/fates#1350 is integrated and the fates tag is updated with CTSM.

Specific notes

Contributors other than yourself, if any: @rgknox

CTSM Issues Fixed (include github issue #): #3062

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

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

Does this create a need to change or add documentation? Did you do so? No

Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on derecho for intel/gnu and izumi for intel/gnu/nag/nvhpc is the standard for tags on master)

NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).

@glemieux glemieux changed the title Fix FATES electron transport variable being passed Fix FATES electron transport variable being set prior to passing to FATES Apr 10, 2025
@glemieux glemieux marked this pull request as ready for review April 10, 2025 23:31
@glemieux glemieux moved this to In progress - master/b4b-dev in CTSM: Upcoming tags Apr 10, 2025
@glemieux glemieux added FATES A change needed for FATES that doesn't require a FATES API update. test: fates Pass fates test suite before merging labels Apr 10, 2025
@glemieux glemieux mentioned this pull request Apr 10, 2025
Open
@glemieux glemieux requested review from ekluzek and rgknox April 10, 2025 23:45
@glemieux glemieux moved this to Finding Reviewers in FATES Pull Request Planning and Status Apr 11, 2025
@ekluzek ekluzek changed the title Fix FATES electron transport variable being set prior to passing to FATES b4b-dev: Fix FATES electron transport variable being set prior to passing to FATES Apr 14, 2025
@wwieder wwieder added the bfb bit-for-bit label Apr 17, 2025
@samsrabin samsrabin moved this from In progress - master/b4b-dev to Done (non release/external) in CTSM: Upcoming tags Apr 17, 2025
@samsrabin samsrabin moved this from Done (non release/external) to In progress - master/b4b-dev in CTSM: Upcoming tags Apr 17, 2025
@samsrabin samsrabin moved this from In progress - master/b4b-dev to In progress - b4b-dev in CTSM: Upcoming tags Apr 17, 2025
@samsrabin samsrabin changed the title b4b-dev: Fix FATES electron transport variable being set prior to passing to FATES Fix FATES electron transport variable being set prior to passing to FATES Apr 17, 2025
@glemieux glemieux changed the base branch from master to b4b-dev April 23, 2025 20:27
@glemieux glemieux force-pushed the fates-electron-transport-fix branch from 906b5c8 to b93d11b Compare April 23, 2025 23:11
@glemieux glemieux moved this from Finding Reviewers to Final Testing in FATES Pull Request Planning and Status Apr 23, 2025
@glemieux
Copy link
Collaborator Author

glemieux commented Apr 24, 2025 •
edited
Loading

Regression testing on izumi and derecho against ctsm5.3.040 are complete. There are expected NLCOMP fails for all FATES testmods due to pass_electron_transport_model now being present in the lnd_in file.

There is one unexpected failure on derecho in which ERI_D.ne30pg3_t232.I1850Clm60BgcCropG.derecho_intel.clm-clm60cam7LndTuningModeLDust is failing COMPARE_base_hybrid. Looking at the cprnc comparison file, I only see one RMS diff in DPFCT_ROCK:

 478  DPFCT_ROCK   (lndgrid,time)  t_index =      1     1
 479           1    48600  (  5784,     1) ( 36297,     1) ( 35800,     1) ( 35800,     1)
 480                15949   1.030363936379866E+00   2.209353131714683E-01 2.3E-02  4.865174702777264E-01 3.0E-06  4.865174702777264E-01
 481                15949   1.030363936379866E+00   2.209353131714683E-01          4.634836990762434E-01          4.634836990762434E-01
 482                48600  (  5784,     1) ( 36297,     1)
 483           avg abs field values:    5.568803384476952E-01    rms diff: 1.8E-04   avg rel diff(npos):  3.0E-06
 484                                    5.568788942335625E-01                        avg decimal digits(ndif):  1.3 worst:  1.3
 485  RMS DPFCT_ROCK                       1.8239E-04            NORMALIZED  3.2752E-04

All other tests are B4B as expected. As a minor side note, I had to build the following manually as they initially failed out, but upon rebuild and submit, worked in the expected manner:

SMS_Ld5_D_P48x1.f10_f10_mg37.IHistClm60Bgc.izumi_nag.clm-decStart
SMS_D.f10_f10_mg37.I2000Clm60BgcCrop.derecho_nvhpc.clm-crop

izumi: /home/glemieux/ctsm/tests_0422-165243iz
derecho: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr3063-auxclm

@github-project-automation github-project-automation bot moved this from In progress - b4b-dev to In progress - master in CTSM: Upcoming tags Apr 24, 2025
@glemieux
Copy link
Collaborator Author

Note that I've also tested this with NGEET/fates#1350 and the two tests cases I submitted as checks both successful ran to completion, including the new FatesColdElecTransJB testmod.

@glemieux
Copy link
Collaborator Author

glemieux commented Apr 24, 2025 •
edited
Loading

There is one unexpected failure on derecho in which ERI_D.ne30pg3_t232.I1850Clm60BgcCropG.derecho_intel.clm-clm60cam7LndTuningModeLDust is failing COMPARE_base_hybrid. Looking at the cprnc comparison file, I only see one RMS diff in DPFCT_ROCK:

I wasn't sure what could be different between these runs since the updates here don't seem to directly affect dust code. That said, I attempted to re-run the test with the older ctsm_pylib conda environment based on (python v3.7.9) and again with the v3.13.2 update for ctsm5.3.040. The test case run in the older conda environment passed b4b. The re-run in the v3.13.2 conda environment failed again, although this time with a DIFF against baseline and COMPARE_base_rest this time (instead of COMPARE_base_hybird). The diff in both these cases is still only in DPFCT_ROCK.

@samsrabin I'm curious if you'll see this with the bfb-dev tests. I'm wondering if its an unstable testmod or if there is something wrong with my conda env and if I should rebuild it.

old conda: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr3063-dusteri
new conda: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr3063-dusteri-newconda

@samsrabin
Copy link
Member

samsrabin commented Apr 24, 2025 •
edited
Loading

@glemieux ERI_D.ne30pg3_t232.I1850Clm60BgcCropG.derecho_intel.clm-clm60cam7LndTuningModeLDust worked for me at commit c29f0cd42b10c589adffa7b140e882d2fe8ba1f4.

/glade/derecho/scratch/samrabin/tests_0424-125031de/ERI_D.ne30pg3_t232.I1850Clm60BgcCropG.derecho_intel.clm-clm60cam7LndTuningModeLDust.GC.0424-125031de_int/

@glemieux
Copy link
Collaborator Author

@glemieux ERI_D.ne30pg3_t232.I1850Clm60BgcCropG.derecho_intel.clm-clm60cam7LndTuningModeLDust worked for me at commit c29f0cd42b10c589adffa7b140e882d2fe8ba1f4.

/glade/derecho/scratch/samrabin/tests_0424-125031de/ERI_D.ne30pg3_t232.I1850Clm60BgcCropG.derecho_intel.clm-clm60cam7LndTuningModeLDust.GC.0424-125031de_int/

Thanks. I'll try using that commit with both my conda environments next.

@glemieux glemieux moved this from Final Testing to Stuck in FATES Pull Request Planning and Status Apr 24, 2025
@glemieux
Copy link
Collaborator Author

glemieux commented Apr 25, 2025 •
edited
Loading

After verifying that I was getting b4b results with c29f0cd, I decided to delete my local branch of this PR on derecho and fetch it again. Retesting with the branch, which is using the same commit b93d11b (doubling checking via SRCROOT_GIT_STATUS), I'm somehow now getting b4b results for ERI_D.ne30pg3_t232.I1850Clm60BgcCropG.derecho_intel.clm-clm60cam7LndTuningModeLDust 🤷.

@glemieux glemieux mentioned this pull request Apr 28, 2025
Merged
@glemieux glemieux changed the base branch from b4b-dev to master April 28, 2025 22:38
@glemieux glemieux force-pushed the fates-electron-transport-fix branch from b93d11b to 303fd47 Compare April 28, 2025 23:23
@glemieux
Copy link
Collaborator Author

Note that this branch has now be merged into #3058.

@samsrabin samsrabin added the done Issues whose closing PR is done but not yet merged (pending test re-run ok) label May 1, 2025
@glemieux glemieux moved this from Stuck to Final Testing in FATES Pull Request Planning and Status May 5, 2025
@glemieux glemieux moved this from Final Testing to Hold in FATES Pull Request Planning and Status May 5, 2025
@adrifoster adrifoster merged commit 2f61b30 into ESCOMP:master May 9, 2025
3 checks passed
@github-project-automation github-project-automation bot moved this from Hold to Ready to Integrate in FATES Pull Request Planning and Status May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgknox rgknox rgknox approved these changes

@ekluzek ekluzek Awaiting requested review from ekluzek

Assignees

@glemieux glemieux

Labels
bfb bit-for-bit done Issues whose closing PR is done but not yet merged (pending test re-run ok) FATES A change needed for FATES that doesn't require a FATES API update. test: fates Pass fates test suite before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect value being set for FATES electron transport namelist option
5 participants
@glemieux @samsrabin @rgknox @wwieder @adrifoster