Skip to content

Add default science configurations#44

Merged
ccarouge merged 3 commits intomasterfrom
23-implement-default-science-configurations
Mar 27, 2023
Merged

Add default science configurations#44
ccarouge merged 3 commits intomasterfrom
23-implement-default-science-configurations

Conversation

@SeanBryan51
Copy link
Copy Markdown
Collaborator

This change modifies the behaviour so that when a science configuration file is not provided by the user, benchcab will default to using internally defined science configurations.

Addresses issue #23

@SeanBryan51 SeanBryan51 linked an issue Mar 21, 2023 that may be closed by this pull request
@SeanBryan51 SeanBryan51 requested a review from ccarouge March 21, 2023 23:34
This change modifies the behaviour so that when a
science configuration file is not provided by the user,
benchcab will default to using internally defined science
configurations.

Addresses issue #23
@SeanBryan51 SeanBryan51 force-pushed the 23-implement-default-science-configurations branch from a59023c to 9e72a93 Compare March 22, 2023 00:00
Copy link
Copy Markdown
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

What is here is good. But I would add a global attribute to the netcdf output files. Maybe add the attribute "IS_DEFAULT_CONFIG" and the values could be: "DEFAULT_CONFIG_SET" or "CUSTOM_CONFIG_SET". Feel free to find better names/values.

I know the config is already listed but that's easier to see and find this way.

@SeanBryan51
Copy link
Copy Markdown
Collaborator Author

What is here is good. But I would add a global attribute to the netcdf output files. Maybe add the attribute "IS_DEFAULT_CONFIG" and the values could be: "DEFAULT_CONFIG_SET" or "CUSTOM_CONFIG_SET". Feel free to find better names/values.

I know the config is already listed but that's easier to see and find this way.

Yep we should do this for reproducibility reasons. However this made me realise, I think we should avoid having command line options that might effect the results of the test because:

  • these CLI options would have to be specified in the output (as we are planning on doing now) which might clutter the netcdf global attributes over time
  • in practice, it might be a pain to work out what command line arguments were used to reproduce a test (we would have to get access to the run directory and inspect their netcdf output to see how benchcab was run)
  • ideally, the config file should contain all benchcab related settings so everything is reproducible solely from the config

So instead of specifying science configs as a command line argument, we add it to the config. What do you think?

@ccarouge
Copy link
Copy Markdown
Member

I agree, the more we can put in the config.yaml, the better.

This change lets the user provide an optional
`science_configurations` key in config.yaml and removes
the `science_config` command line option. This is to
improve test reproducibility. See:
#44 (comment)

Fixes #42
Comment thread tests/common.py
},
"science_configurations": {
"sci0": {
"cable": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Be careful! This breaks adjust_namelist_file for now since it expects the configs without the cable level. But leave it in since that's coming with the next PR.

@ccarouge ccarouge merged commit 58080ed into master Mar 27, 2023
@SeanBryan51 SeanBryan51 deleted the 23-implement-default-science-configurations branch March 27, 2023 09:38
SeanBryan51 added a commit to CABLE-LSM/bench_example that referenced this pull request Mar 27, 2023
Move science configurations from site_configs.yaml to
config.yaml. This change is a part of
CABLE-LSM/benchcab#44

Specify `cable` in the top level of each science config
dictionary. This change is a part of
CABLE-LSM/benchcab#48
SeanBryan51 added a commit to CABLE-LSM/bench_example that referenced this pull request Mar 27, 2023
Move science configurations from site_configs.yaml to
config.yaml. This change is a part of
CABLE-LSM/benchcab#44

Specify `cable` in the top level of each science config
dictionary. This change is a part of
CABLE-LSM/benchcab#48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Typo: make_barbones_science_config should be make_barebones_science_config Implement default science configurations

2 participants