Closed gunterl closed 6 years ago
@gunterl have you gotten confirmation from @whlipscomb on the removal of a bunch of these defaults from the cism.config file? I'm pretty sure that he has asked for many of these to appear in the default cism.config file in the past, so I want to check that he's on-board with this removal. Maybe we should have a brief chat about this.
I also want to add: this removal of defaults from cism.config is contrary to the trend in other components: the general movement is towards listing all relevant namelist values explicitly in namelist files. Let's talk more later.
Also, @gunterl , a more general comment: thank you very much for reviewing this!
@billsacks and @gunterl, I'm generally in favor of listing more rather than fewer defaults. Apologies if I misunderstood something on my call with Gunter yesterday. I appreciate the review too!
Maybe we can check in on the phone this morning (after the LIWG call) and make sure we're on the same page with respect to what info goes in which file, and how we handle the cases where the CESM default agrees with or differs from the CISM default, respectively.
I have a meeting right after the liwg call, but we can check in later in the morning.
Just to record what we discussed. We agreed with this statement that I made here: https://github.com/NCAR/cism-development/pull/14#issuecomment-387752213
My question is whether, as a rule, we list in the default file only the CESM defaults that differ from the CISM defaults, or rather list all the defaults we think are important in some sense for CESM (and hence might be adjusted by users).
We haven't been 100% consistent here, but we've been much closer to
list all the defaults we think are important in some sense for CESM (and hence might be adjusted by users)
Ones we've left out have generally either been (a) irrelevant for most CESM runs, or (b) so obscure that we've felt that the benefit of listing them in the default config file was outweighed by the cost of needing to keep them up-to-date if changes are made to the desired default value. (However, in the future, if we want to move in the same direction as other CESM components, then I think (b) is no longer a good rule.)
@gunterl will either modify this PR or open a new PR reverting most of the changes to the namelist defaults file.
Thanks so much for this, @gunterl ! I have made a few other minor changes accompanying yours. If you're interested (for future reference) in seeing what else needs to be changed in general, you can look at the commits I just pushed to this branch: 9b285b23f8ec118da8b59ad3ad23ed431df06969 and 6dfeaedb2b1518fd8f56c8cd5abaa6a5be76483a . Don't spend too much time trying to understand the build-namelist script, though, because sometime soon I'd like to bring in a python rewrite that Mariana started writing at one point.
I'm attaching the latest generated version of the cism.config
file in case you and/or @whlipscomb would like to do a final review of this final version.
@gunterl, thanks for taking care of this long and complex file. I made a list of suggestions, some of which are unrelated to the most recent work but probably should be updated. Line numbers may not be exact, but should be in the ballpark:
486: Options 3 and 5 have new meanings; see glide_types 508: Change "strain rate criterion" to "stress criterion" 510: Add '[NOT SUPPORTED]' to option 9 576: Change "local SIA" to "glissade local SIA (which_ho_approx = -1)" 587: You might note that evolution options 0, 1 and 2 are for glide only, while 3, 4 and 5 are for glissade only 685, 698: affect -> affects 760: profile is a glide-only option 821: Units of thck_gradient_ramp are meters 845: marine_margin = 3 and 4, not 2 and 4 855: marine_margin = 2 only 860: taumax_cliff is glissade only 883, 893: Eigencalving will be supported (at least, that's the plan for Miren et al.). OK to stick with hard-coded CISM defaults for now. You can say these parameters are for marine_margin = 7 and 8 only. 981: basal_tract_const is relevant only when slip_coeff is relevant (glide or glissade local SIA) 1148: relevant only for which_ho_effecpress = 2 1157, 1166: relevant only for which_ho_bwat = 2 1297: Option descriptions are out of date. See glide_types. 1452: Since this is an expert setting, should it use the hard-coded CISM default?
Thanks again. CISM has become a pretty complex model, so you're drinking out of a firehose. I will look at the namelist defaults later this evening.
@whlipscomb https://github.com/whlipscomb which_ho_ground_bmelt is listed as expert setting in the namelist definition file but was also written in the default namelist file. I would suggest removing the note about "expert setting" if that's ok with you.
On Wed, May 9, 2018 at 5:11 PM, William Lipscomb notifications@github.com wrote:
@gunterl https://github.com/gunterl, thanks for taking care of this long and complex file. I made a list of suggestions, some of which are unrelated to the most recent work but probably should be updated. Line numbers may not be exact, but should be in the ballpark:
486: Options 3 and 5 have new meanings; see glide_types 508: Change "strain rate criterion" to "stress criterion" 510: Add '[NOT SUPPORTED]' to option 9 576: Change "local SIA" to "glissade local SIA (which_ho_approx = -1)" 587: You might note that evolution options 0, 1 and 2 are for glide only, while 3, 4 and 5 are for glissade only 685, 698: affect -> affects 760: profile is a glide-only option 821: Units of thck_gradient_ramp are meters 845: marine_margin = 3 and 4, not 2 and 4 855: marine_margin = 2 only 860: taumax_cliff is glissade only 883, 893: Eigencalving will be supported (at least, that's the plan for Miren et al.). OK to stick with hard-coded CISM defaults for now. You can say these parameters are for marine_margin = 7 and 8 only. 981: basal_tract_const is relevant only when slip_coeff is relevant (glide or glissade local SIA) 1148: relevant only for which_ho_effecpress = 2 1157, 1166: relevant only for which_ho_bwat = 2 1297: Option descriptions are out of date. See glide_types. 1452: Since this is an expert setting, should it use the hard-coded CISM default?
Thanks again. CISM has become a pretty complex model, so you're drinking out of a firehose. I will look at the namelist defaults later this evening.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/cism-wrapper/pull/13#issuecomment-387902120, or mute the thread https://github.com/notifications/unsubscribe-auth/AH0K-IdMQy1BneIwLxsUgxuT4o1ntpkZks5tw3eHgaJpZM4T3vTv .
@gunterl, I looked over the namelist defaults file, and it seems fine. Just a couple of minor suggestions:
ll. 211,215: My current favorite diagnostic point on the 4 km grid is (134,280), where instabilities seem most likely. So I'd suggest we switch from (95,285) to this new point.
l. 264: Not a big deal, but basal_tract_const is listed among some unrelated pseudo-plastic parameters. A more logical place might be just after pmp_offset.
I'm OK with removing the "expert setting" note for which_ho_ground_bmlt, as you suggested.
Thanks again for taking care of these updates!
@whlipscomb https://github.com/whlipscomb, I added your suggestions to the files and just pushed them to the repo.
Thanks for double checking on all of this.
@billsacks https://github.com/billsacks, thanks for all your pointers. I will look at the cism config file tomorrow that gets created and get more familiar on running CISM within CESM. I will do some TG compset testing as well.
On Wed, May 9, 2018 at 9:46 PM, William Lipscomb notifications@github.com wrote:
@gunterl https://github.com/gunterl, I looked over the namelist defaults file, and it seems fine. Just a couple of minor suggestions:
ll. 211,215: My current favorite diagnostic point on the 4 km grid is (134,280), where instabilities seem most likely. So I'd suggest we switch from (95,285) to this new point.
l. 264: Not a big deal, but basal_tract_const is listed among some unrelated pseudo-plastic parameters. A more logical place might be just after pmp_offset.
I'm OK with removing the "expert setting" note for which_ho_ground_bmlt, as you suggested.
Thanks again for taking care of these updates!
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/cism-wrapper/pull/13#issuecomment-387943599, or mute the thread https://github.com/notifications/unsubscribe-auth/AH0K-CUGblop-HuQ5QWt4zTX-O5YerbXks5tw7gVgaJpZM4T3vTv .
Thanks, @whlipscomb and @gunterl for your careful review and edits of these files! I know this was painstaking and tedious and I really appreciate it! I'm kicking off testing with the new version now.
This revision is specifically meant for the release of CISM 2.1.
In namelist_definition: we tried our best to be consistent in our description and stating the default value(s) for each entry.
In namelist_default: we removed the reference to default for entries which default are hard-coded in CISM.