Call the new FATES-side FatesReadParameters (removing FATES->CTSM call)

[johnpaulalex]

Description of changes

Have CTSM use the new code path in FATES (not checked in yet) that allows passing in a fates_param_reader_type, which does the actual work reading the parameter files, in lieu of calling CTSM methods.

Associated FATES side PR: NGEET/fates#1096

Specific notes

I ran ERS_D_Ld5.f45_f45_mg37.I2000Clm50FatesRs.cheyenne_intel.clm-FatesCold with the original PR (first commit).
Now running 'fates' test suite against the current PR (both commits).

Contributors other than yourself, if any: @adrifoster

CTSM Issues Fixed (include github issue #):
Some work towards: #2006
Some work on the FATES issue: NGEET/fates#1073

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

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

Testing performed, if any: aux_clm and fates

[rgknox]

@johnpaulalex , these changes are super exciting, thanks for working on this.

When the FATES parameter reads were first refactored, several years ago, there was a design objective in there that allows the user to put the FATES parameters in either the dedicated fates parameter file, or they can put them in the CTSM parameter file. Here is an example of where this optioning happens:

CTSM/src/utils/clmfates_paraminterfaceMod.F90

Lines 67 to 71 in a851a75

	is_host_file = .false.
	call ParametersFromNetCDF(fates_paramfile, is_host_file, fates_params)
	is_host_file = .true.
	call ParametersFromNetCDF(paramfile, is_host_file, fates_params)

But this feature never gained any traction, and many of us now argue that this would just end up making things complicated if different users put their parameters in different files.

My question to you, is would it make the whole parameter API simpler and more straightfoward if we ditched this flexibility? (ie just assume fates parameters are in the fates parameter file). If so, is it worth the effort in ditching, or is it easier to keep it in there?

[johnpaulalex]

Hey Ryan,

Yeah Adrianna mentioned that it wasn't much used.

Practically speaking, I have no strong feelings about it, it's not a big deal to keep the boolean nor ditch it.

Design-wise, though, if we did want such a feature :), I don't think FATES ought to know anything about it -- I think FATES ought to just ask for parameter names (which it already does), and it should be up to the HLM to decide how to access each parameter. (e.g. I could imagine each HLM having an internal map of parameter name to data filename). I guess that's another argument for removing the booleans.

[rgknox]

@johnpaulalex was the plan to remove the (clm-side) FatesReadParameters() routine (and the call in fates) during this set of changes, or at a later date? It is being deprecated, correct?

https://github.com/ESCOMP/CTSM/pull/2198/files#diff-1b6393c47d78f770b0317563f0d65fc4a9c11ffaafe7f4c6f0915e95beb13182R46

[johnpaulalex]

Yes, I wasn't going to remove it yet because FATES still calls it (and that code path will still be executed if the host is E3SM). So I thought I'd remove it once FATES stopped calling it.

(related Fortran question: given a line saying "call x()", I'm guessing the compiler will complain if it can't find the definition of x(), even if in practice that code path is never run?)

[ekluzek]

@johnpaulalex on your Fortran question. The build will fail at the link step if it can't find the function that's needed, even if at run it wouldn't be used.

We do sometimes use C preprocessing syntax for things like this if it's the best way to handle it.

So adding a

#ifdef

Type syntax around it. We just avoid that if possible.

But undoubtedly there is a CPP token that could be checked to see if you are CESM or E3SM.

[rgknox]

@johnpaulalex we can synchronize this pull request with the FATES pull request so they come in together (E3SM will continue to point to the current hash of fates, so that is ok too), so if you wish you can remove the call. We would then update the API major number on the fates tag so people realize there is a non-backwards compatible change that was introduced.

[johnpaulalex]

Thanks @rgknox. If I could wrap the pre-existing call (on the fates side) to the HLM-side FatesReadParameters() with some '#ifdef E3SM' like @ekluzek is suggesting, then I could do it in one shot (i.e. remove the CTSM-side FatesReadParameters in this PR).

But...I'm not sure such a thing exists. Several questions:

• I don't see any #ifdef's in FATES - are we sure that all its files will be run through the C preprocessor?
• I went poking around the CTSM code looking for possible #defines. There's only four, in the lilac subdirectory[1] and I'm guessing they're only active in the file they're defined in, so that approach wouldn't help here anyway. The more likely avenue is a Makefile defining them on the fortran compiler's command line; maybe something like [2] but that Makefile doesn't look like it's for all of CTSM (any idea where that is, if there is one?). I also looked for pre-existing #ifdef's, there's about 6 files with them[3], but none of those values looked promising.
• I also looked at E3SM-Project/E3SM (is that the right repo?) - loads of #defines [4] but again I suspect they're only file-local. But also loads of #ifdefs [5] - this seemed more promising. And lots of defines in Makefiles: [6]. Nothing immediately looked like it would definitely be defined unconditionally for all E3SM builds, but maybe I missed something?

Otherwise, I guess we could just add one on one side's Makefile - does anybody know how (where) to do that? Also, either way, I couldn't validate that my PR would compile correctly for the E3SM side - I guess, Ryan, maybe I'd have to ask you to do it?

John

[1] https://github.com/search?q=repo%3AESCOMP%2FCTSM%20%23define&type=code
[2] https://github.com/ESCOMP/CTSM/blob/master/tools/mkprocdata_map/src/Makefile.common#L152
[3] https://github.com/search?q=repo%3AESCOMP%2FCTSM+%23ifdef&type=code
[4] https://github.com/search?q=repo%3AE3SM-Project%2FE3SM+%23define+path%3A**.F90&type=code
[5] https://github.com/search?q=repo%3AE3SM-Project%2FE3SM+%23ifdef+path%3A**.F90&type=code&p=1
[6] https://github.com/search?q=repo%3AE3SM-Project%2FE3SM+CPPFLAGS&type=code

[johnpaulalex]

Oh, and following on to my previous comment: I don't know if it's worth doing this #ifdef thing just to skip a relatively simple follow-up PR (but I acknowledge that's still more work for all y'all to integrate it, test, and submit) - but I'm happy to go either way on it.

[rgknox]

@johnpaulalex , sorry I should had mentioned the ifdef thing earlier. We do not allow ifdefs and other pre-processor pragmas in fates, because we don't want to set that requirement for other host models that use fates

[rgknox]

But per your questions, I also don't think we need to use the ifdefs, we just make the changes on CTSM and FATES simultaneously, and we merge them into the main branches at roughly the same time. When we do this, we merge the FATES side first. Then we make a new fates tag, and then we merge the ctsm side which points to the new tag. Because of this order of operations, a user will never be caught in limbo. This is true for the E3SM users as well. That model will continue to point to an older version of fates until we make the changes on the E3SM side. @ekluzek, @adrifoster @glemieux do you see it the same way (or am I missing something)?

[adrifoster]

But per your questions, I also don't think we need to use the ifdefs, we just make the changes on CTSM and FATES simultaneously, and we merge them into the main branches at roughly the same time. When we do this, we merge the FATES side first. Then we make a new fates tag, and then we merge the ctsm side which points to the new tag. Because of this order of operations, a user will never be caught in limbo. This is true for the E3SM users as well. That model will continue to point to an older version of fates until we make the changes on the E3SM side. @ekluzek, @adrifoster @glemieux do you see it the same way (or am I missing something)?

I think it's fine to do it the way we normally do things (i.e. basically make all changes on the FATES and CTSM/E3SM sides at the "same" time). the way @johnpaulalex is doing it here does make it more incremental, but it's true because of the way we use the externals file I believe we are "safe" as long as FATES brings in the PR first, then CTSM/E3SM does a PR with the required changes in the code, PLUS updating the Externals_CLM.cfg to reference the correct new FATES tag.

[johnpaulalex]

ahh, I didn't grok how you could do all that. sgtm. I've updated both this PR and the FATES one accordingly, all the old code paths are gone. I'm rerunning the 'fates' test suite to confirm it's happy.

[glemieux]

+1 @adrifoster and @rgknox assessment. I should note that I started up the e3sm-side PR adapting @johnpaulalex work: E3SM-Project/E3SM#6027 which will update the fates submodule commit hash so that it points to the new fates tag once we bring it in. I've pointed it at John's fates branch for now and have started up build testing.

[adrifoster]

All fates and aux_clm tests pass

[ekluzek]

@adrifoster and @johnpaulalex thanks to you both for working on this! I'm really excited about this improvement. It's an incremental step towards larger improvements to the FATES/HLM interface, but a nice first step that demonstrates an advancement. It's also a positive to have it done in small steps so we can see continuous improvement.