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

Define new fates_param_reader_type type to abstract HLM-side param I/O #1096

Merged
merged 6 commits into from
Nov 17, 2023

Conversation

johnpaulalex
Copy link
Contributor

@johnpaulalex johnpaulalex commented Oct 13, 2023

Also copy FatesReadParameters() over from CTSM. (The HLM-side ones can be deleted after they switch over to using this new one, coming next).

Description:

FATES should not call the HLM, but currently it calls an HLM-side FatesReadParameters() which calls an HLM-side ParametersFromNetCDF() method to actually read the parameters from disk.

With this change, FATES now has its own copy of FatesReadParameters() which takes in an HLM-provided 'fates_param_reader_type' to read the parameters from disk. Its existing SetFatesGlobalElements1() method now takes in an optional fates_param_reader_type. Nobody passes this optional param in, yet.

Upcoming PR's will modify the HLM's to construct and pass in instances of fates_param_reader_type (which will basically call the HLM-side ParametersFromNetCDF() method), and then remove the old code path.

Associated CTSM PR: ESCOMP/CTSM#2198
Associated E3SM PR: E3SM-Project/E3SM#6027

Issue ESCOMP/CTSM/2006

Collaborators:

adrifoster@

Expectation of Answer Changes:

No-op; this adds a code path that isn't yet used by any HLM.

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:
CTSM ctsm5.1.dev142

FATES baseline hash-tag:
fates-sci.1.67.5_api.27.0.0

Test Output:
(I ran the 'fates' test suite)

sh /glade/scratch/jpalex/tests_1011-184611ch/cs.status.fails

1011-184611ch_gnu: 3 tests
FAIL ERS_Lm13.f10_f10_mg37.I2000Clm50Fates.cheyenne_gnu.clm-FatesCold COMPARE_base_rest (EXPECTED FAILURE)

1011-184611ch_int: 34 tests
PEND ERP_P72x2_Ld30.f45_f45_mg37.I2000Clm50FatesRs.cheyenne_intel.clm-mimicsFatesCold SHAREDLIB_BUILD (EXPECTED FAILURE)
FAIL ERS_D_Ld5.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdHydro COMPARE_base_rest (EXPECTED FAILURE)
FAIL ERS_Lm12.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesFireLightningPopDens COMPARE_base_rest (EXPECTED FAILURE)
FAIL ERS_Lm13.f45_f45_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdNoComp COMPARE_base_rest (EXPECTED FAILURE)

… practice, all callers use the FatesParametersInterface::RegisterParameter() default of false). Note this parameter is also called sync_with_host elsewhere.
johnpaulalex and others added 4 commits November 1, 2023 12:27
… time, and E3SM will not reference this new FATES version until it's also updated.
@adrifoster
Copy link
Contributor

All tests on cheyenne pass except for expected fails:

aux_clm:

1116-092112ch_int: 117 tests
    PEND ERP_P72x2_Ld30.f45_f45_mg37.I2000Clm50FatesRs.cheyenne_intel.clm-mimicsFatesCold SHAREDLIB_BUILD (EXPECTED FAILURE)

 
1116-092112ch_nvh: 3 tests
    FAIL SMS_D.f10_f10_mg37.I2000Clm51BgcCrop.cheyenne_nvhpc.clm-crop RUN time=20 (EXPECTED FAILURE)
    FAIL SMS.f10_f10_mg37.I2000Clm50BgcCrop.cheyenne_nvhpc.clm-crop RUN time=445 (EXPECTED FAILURE)
    FAIL SMS.f45_f45_mg37.I2000Clm51FatesSpRsGs.cheyenne_nvhpc.clm-FatesColdSatPhen RUN time=19 (EXPECTED FAILURE)

fates

1116-091452ch_gnu: 5 tests
    PASS ERS_Lm13.f10_f10_mg37.I2000Clm50Fates.cheyenne_gnu.clm-FatesCold COMPARE_base_rest (UNEXPECTED: expected FAIL)
    FAIL PEM_D_Ld15.5x5_amazon.I2000Clm50FatesRs.cheyenne_gnu.clm-FatesColdSeedDisp COMPARE_base_modpes (EXPECTED FAILURE)

 
1116-091452ch_int: 35 tests
    PEND ERP_P72x2_Ld30.f45_f45_mg37.I2000Clm50FatesRs.cheyenne_intel.clm-mimicsFatesCold SHAREDLIB_BUILD (EXPECTED FAILURE)
    FAIL ERS_D_Ld5.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdHydro COMPARE_base_rest (EXPECTED FAILURE)
    PASS ERS_Lm12.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesFireLightningPopDens COMPARE_base_rest (UNEXPECTED: expected FAIL)
    PASS ERS_Lm13.f45_f45_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdNoComp COMPARE_base_rest (UNEXPECTED: expected FAIL)

@adrifoster
Copy link
Contributor

fates tests on izumi also pass except for expected fails:

1116-112732iz_nag: 8 tests
    PEND ERS_D_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.izumi_nag.clm-FatesColdPRT2 RUN (UNEXPECTED: expected FAIL)
    FAIL ERS_D_Ld5.1x1_brazil.I2000Clm50FatesCruRsGs.izumi_nag.clm-FatesColdHydro COMPARE_base_rest (EXPECTED FAILURE)

@adrifoster
Copy link
Contributor

izumi had two failing diffs for baselines but @slevis-lmwg and I think this is actually because we got new driver data for our NEON NIWOT data (which we automatically download from the NEON server for each new run) in between his dev152 baselines yesterday and my test today

Running some tests to confirm this theory now...

@glemieux
Copy link
Contributor

Thanks @adrifoster. I've got e3sm tests going on perlmutter as well, to make sure things look ok coupling to that side.

As an aside, it looks like #1098 fixed issues #897 and ESCOMP/CTSM#667 🎉. Would you mind removing those from the expected failures list?

@glemieux glemieux mentioned this pull request Nov 16, 2023
3 tasks
@adrifoster
Copy link
Contributor

All fates and aux_clm tests now pass!

@glemieux glemieux merged commit 0b105be into NGEET:main Nov 17, 2023
1 check passed
peterdschwartz added a commit to E3SM-Project/E3SM that referenced this pull request Feb 6, 2024
…tor' into next (PR #6027)

This PR refactors the elm-fates interface to avoid having fates call elmfates_interfacemod.
FatesReadParameters has been moved from the aforementioned module into FATES and takes in an
HLM-provided fates_param_reader_type to read the parameters from disk.
The existing SetFatesGlobalElements1 method now takes in an optional fates_param_reader_type.

Upcoming PR's will modify the HLM's to construct and pass in instances of fates_param_reader_type
(which will basically call the HLM-side ParametersFromNetCDF method), and then remove the old code path.

This work is ported from ESCOMP/CTSM#2198 and is associated with NGEET/fates#1096

Fixes #6029

[BFB]
peterdschwartz added a commit to E3SM-Project/E3SM that referenced this pull request Feb 7, 2024
…tor' (PR #6027)

This PR refactors the elm-fates interface to avoid having fates call elmfates_interfacemod.
FatesReadParameters has been moved from the aforementioned module into FATES and
takes in an HLM-provided fates_param_reader_type to read the parameters from disk.
The existing SetFatesGlobalElements1 method now takes in an optional fates_param_reader_type.

Upcoming PR's will modify the HLM's to construct and pass in instances of fates_param_reader_type
(which will basically call the HLM-side ParametersFromNetCDF method), and then remove the old code path.

This work is ported from ESCOMP/CTSM#2198 and is associated with NGEET/fates#1096

Fixes #6029

[BFB]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants