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.

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

Refactor parameter read procedure in elmfates API to avoid FATES calling hlm-side procedures #6027

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Refactor parameter read procedure in elmfates API to avoid FATES calling hlm-side procedures #6027

merged 2 commits into from
Feb 7, 2024

Conversation

glemieux
Copy link
Contributor

@glemieux glemieux commented Oct 27, 2023 •
edited by peterdschwartz
Loading

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]

@glemieux glemieux mentioned this pull request Oct 27, 2023
Merged
4 tasks
@glemieux glemieux changed the title [WIP] Refactor elmfates API to avoid calling hlm-side procedures [WIP] Refactor parameter read procedure in elmfates API to avoid FATES calling hlm-side procedures Oct 27, 2023
@rljacob rljacob added the Land label Oct 27, 2023
@glemieux glemieux mentioned this pull request Nov 1, 2023
Merged
@glemieux glemieux changed the title [WIP] Refactor parameter read procedure in elmfates API to avoid FATES calling hlm-side procedures Refactor parameter read procedure in elmfates API to avoid FATES calling hlm-side procedures Nov 17, 2023
@glemieux
Copy link
Contributor Author

This should come in after #6018

@glemieux
Copy link
Contributor Author

glemieux commented Nov 17, 2023 •
edited
Loading

Initial testing against my baseline using sci.1.67.5_api.27.0.0 and 0b367fb are b4b, with the expected exception of issue NGEET/fates#1106 (which hasn't been integrated yet).

@glemieux
Copy link
Contributor Author

glemieux commented Nov 17, 2023 •
edited
Loading

Incorporate FATES-side fix for #6029

@glemieux glemieux force-pushed the lnd/fates-api-readparams-refactor branch from fd43e2b to 5e1f47e Compare November 22, 2023 00:50
@glemieux
Copy link
Contributor Author

@peterdschwartz this is ready to review

@glemieux glemieux mentioned this pull request Dec 4, 2023
Merged
@rljacob rljacob added the FATES label Dec 6, 2023
@rljacob
Copy link
Member

rljacob commented Dec 7, 2023

Waiting on #6018

@glemieux glemieux force-pushed the lnd/fates-api-readparams-refactor branch from d7825fb to f15d72b Compare January 8, 2024 18:53
@glemieux glemieux force-pushed the lnd/fates-api-readparams-refactor branch from f15d72b to a9de076 Compare January 23, 2024 23:29
@glemieux glemieux force-pushed the lnd/fates-api-readparams-refactor branch from a9de076 to 43d7472 Compare February 1, 2024 18:08
@rljacob
Copy link
Member

rljacob commented Feb 1, 2024

#6018 was merged so this can proceed.

@rljacob rljacob closed this Feb 1, 2024
@rljacob rljacob reopened this Feb 1, 2024
@peterdschwartz
Copy link
Contributor

Sorry I missed the meeting. @glemieux Is this still ready to review after that rebase?

@glemieux
Copy link
Contributor Author

glemieux commented Feb 1, 2024

@peterdschwartz thanks for checking in. Yes, it's ready for review. I've got updated tests running on perlmutter. Hopefully should have results on Friday.

@glemieux
Copy link
Contributor Author

glemieux commented Feb 2, 2024 •
edited
Loading

All tests in the e3sm_land_developer are B4B against the master baseline. One test failed RUN, which also appears to be failing via the dashboard: SMS.r05_r05.I1850ELMCN.pm-cpu_intel.elm-qian_1948

All regression tests for the newly expanded fates test list are B4B against the baseline I generated using master.

@peterdschwartz
Copy link
Contributor

Everything looked good after testing on chrysalis so I'll try to merge this today.

peterdschwartz added a commit that referenced this pull request Feb 6, 2024
@peterdschwartz
Merge remote-tracking branch 'glemfork/lnd/fates-api-readparams-refac…
e69105f 
…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
Copy link
Contributor

merge to next

@peterdschwartz peterdschwartz merged commit 67abd00 into E3SM-Project:master Feb 7, 2024
4 checks passed
@peterdschwartz
Copy link
Contributor

merged to master

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

@peterdschwartz peterdschwartz peterdschwartz approved these changes

@rgknox rgknox Awaiting requested review from rgknox

Assignees

@peterdschwartz peterdschwartz

Labels
FATES Land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build error with AMD compiler in FatesLitterMod.F90
4 participants
@glemieux @rljacob @peterdschwartz @bishtgautam