Interface: endrun and cleaning of fates<->hlm globals #180
Conversation
…ls. Fix was trivial, both branches added a subroutine there, both subroutines are still relevant and orthogonal.
… did some light cleaning of some other calls to the cime shares, mostly just for readability.
…d having it a dependent variable that goes on to set HLM allocations. This pass also simplifies how HLM cohort<->grid,lu and column mappings are calculated. This is done by removing ed_cohort_vector. In this phase we have not removed that memory structure, but we are bypassing it.
…n this phase. The difference is that scalars dictated by the hlm going into fates are now living in FatesInterface mod where their wrappers are. Likewise, the varialbes that are dictated by fates and are destined for the HLM are also in this module, as are the wrapper call functions. The wrapper call functions to set fates_maxElementsPerPatch and fates_maxElementsPerSite are called during Initialize1(), and fates_maxElementsPerSite is now used during decompInit instead of the ed_vec_cohort structure.
…ning. Combined clm_fates%init and clm_fates%init_allocate. Created a trivial cohort dimensioning scheme for non-fates runs.
| end subroutine SetFatesTime | ||
| if (present (msg)) then | ||
| write(fates_log(),*)'ENDRUN:', msg | ||
| else |
There was a problem hiding this comment.
I've never cared for this convention in clm. It shifts the burden of understanding why endrun was called from the developer who called endrun onto the user. The original developer understands why they are inserting the endrun call and should be required to provide that information to the user in the message string.
There was a problem hiding this comment.
Good point. But, just to double check, is your suggestion that we remove the optional use of providing a message, and make it mandatory? Or do you want us to require more structure in the message?
There was a problem hiding this comment.
I think simply requiring a message is good enough. If you can think of additional structure that is easy to maintain, then that's great too.
|
Testing conflict resolutions and changes that make messages in endrun() mandatory. |
|
All tests on e1b5b9b PASS, including regression tests. |
|
@bandre-ucar : could you sign off on, or suggest improvements to the HLM side changes that I am proposing in this PR? Feel free to bring in others to check too if need be. |
|
No major concerns from the host side. Testing. |
|
Runtime failure on nag: |
|
I think I know the problem, I solved this in another branch I based on this one. I will submit the fix tomorrow. |
|
@rgknox Thanks. Running tests. |
|
nag tests passed. Running on yellowstone. |
|
PR fails to compile under pgi: |
|
I am wondering if PGI didn't like the "use" call to IEEE_ARITHMATIC and therefor dropped the definition of r8 => fates_r8? Or perhaps there is a circular dependency and it was not able to compile FatesConstantsMod before EDAccumulateFluxesMod? I'm removing the use IEEE_Arithmetic, which is no-longer needed and will give it a try on yellowstone PGI. Do you have any foresight if this may be the issue? |
|
@rgknox I looked at it for a second and didn't understand what the problem was. PGI has been the most buggy compiler. If removing ieee doesn't work, you might try using fates_r8 directly or moving the use statement to the module level. I also think the use statement for endrun needs to be updated. |
|
thanks @bandre-ucar, I just caught that rogue endrun() too. I will check back in with the test results. |
…unnecessary call to IEEE arithmetic.
|
I did a couple of changes, 1) moved the definition of r8 => fates_r8 to the module level and 2) removed the definition of IEE_ARITHMETIC. pgi now compiles on yellowstone. I'd be more than happy to fully test the PGI @bandre-ucar , but I don't have a PGI baseline created for master, so your probably a step ahead of me here. Feel free to get the test started if you like, but otherwise I have a PGI baseline for master in the works. |
|
Thanks. I'll start another round of testing. FYI - I keep all the ed-clm-fates baselines on yellowstone in the standard location: /glade/p/cesmdata/cseg/ccsm_baselines Named: ed-clm-{CHANGESET_ID} |
|
great to know, thanks |
|
Yellowstone ed and clm_short testing passed. |
In this change-set roughly two objectives are tackled. 1) a wrapper was created to call the CIME shutdown/end-run utitlities. The wrapper is essentially the same wrapper that CLM uses, but we need our own to break dependence. 2) We have various globals in FATES that are dictated by the HLM, and vice versa. I tried to wrangle these variables into FatesInterfaceMod, and give them some functions to set those variables and protect them. Moreover, the new global fates_maxElementsPerSite and fates_maxElementsPerPatch, are calculate based on the maximum requirements for array allocation for FATES restarts. Previously, this was a hard-coded and misleading value. While FATES asks the HLM to allocate a "cohort" array for restart variables, it is really a multi-purpose array, and may be larger than the needs we have to store cohorts.
Fixes: #178 and #177 and #144, and #186
Addresses: #141
User interface changes?: No
Code review:
Test suite: edTest, clm_short_45
Test baseline: cdb9db7
Test namelist changes: None
Test answer changes: b4b with baseline
Test summary: all PASS