added new coupling mode for noresm#677
Conversation
billsacks
left a comment
There was a problem hiding this comment.
This looks good to me, though I'm curious how you're going to set COUPLING_MODE correctly for noresm and how, if at all, this will interact with CIME_MODEL.
|
This is one thing that needs to be different for cesm and noresm. The default for CMEPS in noresmhub will be noresm and the default for cmeps in ESCOMP will be cesm. So at this point it does not interact with CIME_MODEL - which is a much bigger lift. For noresm - this will still be CIME_MODEL will still be cesm. Hopefully with the CIME refactor it will be easier to pull that out. |
ekluzek
left a comment
There was a problem hiding this comment.
This makes sense to me as well. It all looks correct to me. I didn't see anything of concern in skimming over the code, other than I'm wondering about if some of the logic statements need paranthesis (even if just for clarity).
|
Unless my eyes are playing tricks on me, I think the last two commits were unnecessary: I think these @mvertens as far as I'm concerned, go ahead and merge this when you're ready. |
|
@mvertens - oh, I remembered I was going to send you a test you could run to confirm that this is bit-for-bit for CESM. I'd suggest ERS_Ld5.ne30pg3_t232.B1850C_LTso.derecho_intel.allactive-defaultio You can run this from cesm3_0_alpha09d with this CMEPS update, and compare against the baselines here: /glade/campaign/cesm/cesmdata/cesm_baselines (That's the default baseline root for CESM.) |
|
@mvertens - Actually, I'll just go ahead and run that test for you while I'm thinking of it. |
Crap you are right. I wasn't looking at it right when I made the suggestion. Sorry about that. |
|
I have run @mvertens I'll let you decide when you're ready to merge this, including whether you want to keep the latest commits or revert to your original version before the extra parentheses (if the latter, I'm personally okay with you force pushing to this PR to remove the last two commits from history... and would actually slightly prefer that as a less error-prone approach - something like |
|
@billsacks - thank you for running the test! I have done the force push you suggested - and have backed out the last two commits. This should be ready to be merged. |
|
Do you want to merge this or should I? I'm fine with either. |
13c30fa to
c86256e
Compare
|
@mvertens - sorry: I think I gave you the wrong commands: the |
Description of changes
Added new valid value to noresm to the COUPLING_MODE xml variable
Specific notes
This change permits namelist values for coupling_mode to now include noresm. Conditionals throughout the code now can test for both noresm and cesm.
This xml variable can also now be included in CDEPS config variables to distinguish between noresm and cesm settings.
Not that this PR also will need to be included for upcoming WW3 PRs from noresm to cesm.
Contributors other than yourself, if any:
CMEPS Issues Fixed:
Are changes expected to change answers? bfb
Any User Interface Changes (namelist or namelist defaults changes)? None
Testing performed
ERS_Ld5.ne30pg3_t232.B1850C_LTso.derecho_intel.allactive-defaultiofrom cesm3_0_alpha09d - passes and bit-for-bit. I ran this test both with (a) just the first commit on this branch, and (b) the 3 commits that included Erik's suggested extra parentheses.