-
Notifications
You must be signed in to change notification settings - Fork 133
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.
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
Add support for .cice_set, git clone --depth=1, atmbndy namelist output, script warning messages for --set conflicts #494
Conversation
Add warning messages when multiple set options overlap (CICE-Consortium#243) Fix namelist output diagnostic for atmbndy (CICE-Consortium#493) Modify git clone in script to use --depth=1 (CICE-Consortium#492)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My piece in this looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, requesting just one small correction.
doc/source/user_guide/ug_running.rst
Outdated
@@ -229,6 +232,8 @@ change the defaults. This is done as part of the ``cice.setup`` and the | |||
modifications are resolved in the **cice.settings** and **ice_in** file placed in | |||
the case directory. If multiple options are chosen that conflict, then the last | |||
option chosen takes precedent. Not all options are compatible with each other. | |||
Settings on the command line will take precedent over serttings defined in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be corrected to
"... take precedence over settings ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I had to look it up. I fixed 5 errors in the document. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @apcraig ! This looks good. I noted 2 small things below.
Hint: if you update the PR description to add "Fixes" before each issue number, the issues will be closed automatically when the PR is merged :)
@@ -202,7 +202,10 @@ specifies the compilation environment associated with the machine. This should | |||
specifies the grid. This is a string and for the current CICE driver, gx1, gx3, and tx1 are supported. (default = gx3) | |||
|
|||
``--set``, ``-s`` SET1,SET2,SET3 | |||
specifies the optional settings for the case. The settings for ``--suite`` are defined in the suite file. Multiple settings can be specified by providing a comma deliminated set of values without spaces between settings. The available settings are in **configurations/scripts/options** and ``cice.setup --help`` will also list them. These settings files can change either the namelist values or overall case settings (such as the debug flag). | |||
specifies the optional settings for the case. The settings for ``--suite`` are defined in the suite file. Multiple settings can be specified by providing a comma deliminated set of values without spaces between settings. The available settings are in **configurations/scripts/options** and ``cice.setup --help`` will also list them. These settings files can change either the namelist values or overall case settings (such as the debug flag). For cases and tests (not suites), settings defined in **~/.cice_set** (if it exists) will be included in the --set options. This behaviour can be overridden with the `--ignore-user-set`` command line option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little suggestion to make reviewing documentation easier: one thing we could do is write only one sentence per line. Here there are multiple sentences on the same line so the diff is long, whereas if we'd stick to one sentence per line it would be easy to see that it's two sentences that are added at the end and the rest of the paragraph is not modified. (Whether to do a PR that changes that for the whole documentation is something we could discuss, but maybe just doing it as we go when we make documentation changes could be a good way forward). Also if everybody is on board we should mention it on the wiki page that talks about Documentation changes.
Also, I think the format of the ~/.cice_set
file should be documented: are options separated by commas, just as on the command line (and with no trailing comma) ? Reading the code added in cice.setup
this appears to be the case, but I think it should be mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @phil-blain, I have updated the documentation and added a sentence about the format of the .cice_set file. I also agree about the "long line" issue in the documentation and agree that we can move away from it as we go. I think we have to be a little careful as there may be cases where the line breaks change the formatting, for instance in the indented section highlighted above, but we can also explore that issue moving forward.
Co-authored-by: Philippe Blain <[email protected]>
Co-authored-by: Philippe Blain <[email protected]>
PR checklist
Fix a few different minor issues and features
apcraig
Ran full test suite on gordon with gnu and everything is bit-for-bit. This is primarily script changes.
https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#b055c7f01ad4ad8f5d946d4456188a8f8b8eb55d
Add support for .cice_set (#464)
Add warning messages when multiple set options overlap (#243)
Fix namelist output diagnostic for atmbndy (#493)
Modify git clone in script to use --depth=1 (#492)