Skip to content
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.

Sign up for GitHub

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

Configure option syntax now only uses dashes #358

Merged
merged 3 commits into from
Feb 7, 2025
Merged

Configure option syntax now only uses dashes #358

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
909d835
Change CAM_CONFIG_OPTS arguments to use dashes instead of underscores.
nusbaume Feb 6, 2025
e934bc0
Fix tests broken by config options change.
nusbaume Feb 6, 2025
06e6b2f
Remove single-dash config options, and clean-up argparse calls.
nusbaume Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions cime_config/cam_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,10 @@ def __init__(self, case, case_log):
"To specify the Kessler and Held-Suarez suites as ",
"run time options, use '--physics-suites kessler;held_suarez_1994'."]

#--------------------------
# Set physics_suites string
#--------------------------

self.create_config("physics_suites", phys_desc,
user_config_opts.physics_suites)

Expand Down Expand Up @@ -585,7 +589,7 @@ def parse_config_opts(cls, config_opts, test_mode=False):
[('analytic_ic', False), ('dyn', 'se'), ('dyn_kind', 'REAL64'), ('phys_kind', 'REAL64'), ('physics_suites', 'kessler')]

5. Check that parse_config_opts works as expected when given both a string and logical argument:
>>> config_opts = ConfigCAM.parse_config_opts("--physics-suites kessler --dyn se --analytic_ic")
>>> config_opts = ConfigCAM.parse_config_opts("--physics-suites kessler --dyn se --analytic-ic")
>>> vargs = vars(config_opts)
>>> [(x, vargs[x]) for x in sorted(vargs)]
[('analytic_ic', True), ('dyn', 'se'), ('dyn_kind', 'REAL64'), ('phys_kind', 'REAL64'), ('physics_suites', 'kessler')]
Expand Down Expand Up @@ -618,16 +622,16 @@ def parse_config_opts(cls, config_opts, test_mode=False):
type=str, required=False, default="",
help="""Name of dycore""")

parser.add_argument("--analytic_ic", "-analytic_ic",
parser.add_argument("--analytic-ic", "-analytic-ic",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we still want to support the single-dash versions (e.g., -analytic-ic) that's similar to current CAM if it has changed (from -analytic_ic to -analytic-ic) anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think having just the one dash as an option is ok in case someone typos when manually adding the argument, but if you disagree then I am happy to take it out. Just let me know!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wonder this too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nusbaume! Personally I would be in favor of getting rid of it to have less 'historical' stuff in SIMA that behaves kinda like CAM but not exactly like it (it would be surprising for the user, for example, for -chem something to work like CAM in the future but -analytic_ic to not work). But I am happy either way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with CAM. I only mentioned that in the context of the command line interface where you expect with double dash, more descriptive flags, and with a single dash, a quick, shorthand command. (not always though). I didn't know it's from CAM

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimmielin @boulderdaze those arguments make sense me! I went ahead and removed the single dash options for now (we can always add shorthand options later if desired). I also asked for re-reviews just to make sure everyone is happy, so if you could look over the changes one more time that would be great!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, thank you!

action='store_true', required=False,
help="""Flag to turn on Analytic Initial
Conditions (ICs).""")

parser.add_argument("--dyn_kind", "-dyn_kind",
parser.add_argument("--dyn-kind", "-dyn-kind",
type=str, required=False, default="REAL64",
help="""Fortran kind used in dycore for type real.""")

parser.add_argument("--phys_kind", "-phys_kind",
parser.add_argument("--phys-kind", "-phys-kind",
type=str, required=False, default="REAL64",
help="""Fortran kind used in physics for type real.""")

Expand Down
6 changes: 3 additions & 3 deletions cime_config/config_component.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,10 @@

<!-- Simple models -->
<value compset="_CAM%ADIAB">--physics-suites adiabatic</value>
<value compset="_CAM%TJ16">--physics-suites tj2016 --analytic_ic</value>
<value compset="_CAM%TJ16">--physics-suites tj2016 --analytic-ic</value>
<!-- <value compset="_CAM%KESSLER">-phys kessler -chem terminator -analytic_ic</value> -->
<value compset="_CAM%KESSLER">--physics-suites kessler --analytic_ic</value>
<value compset="_CAM%HS94">--physics-suites held_suarez_1994 --analytic_ic</value>
<value compset="_CAM%KESSLER">--physics-suites kessler --analytic-ic</value>
<value compset="_CAM%HS94">--physics-suites held_suarez_1994 --analytic-ic</value>
<value compset="_CAM%PHYSTEST">--dyn none --physics-suites adiabatic</value>

<!-- Aquaplanet -->
Expand Down
2 changes: 1 addition & 1 deletion cime_config/testdefs/testmods_dirs/cam/outfrq_se_cslam_analy_ic/shell_commands
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
./xmlchange CAM_LINKED_LIBS=" "
./xmlchange --append CAM_CONFIG_OPTS="--analytic_ic"
./xmlchange --append CAM_CONFIG_OPTS="--analytic-ic"