-
Notifications
You must be signed in to change notification settings - Fork 5
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
Isolate config and argument parsing #1086
base: main
Are you sure you want to change the base?
Conversation
53ef44c
to
728b0e2
Compare
@@ -1,7 +1,5 @@ | |||
FLOAT_TYPE: "Float32" | |||
apply_limiter: false | |||
dt: "400secs" |
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.
Removed because 400secs
is the default for dt
and dt_cpl
728b0e2
to
bac5919
Compare
31dbea9
to
c5ba1c7
Compare
c5ba1c7
to
3662d57
Compare
6c02f03
to
c6817f3
Compare
"--dt_seaice" | ||
help = " Sea Ice simulation time step" | ||
"--job_id" | ||
help = "[REQUIRED] A unique identifier for this run" |
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.
Why is this required? We can always make one up if the user doesn't provide it.
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 can add defaults for the config_file
and job_id
, but they'll be mismatched if the user provides one but not the other
"--hourly_checkpoint" | ||
help = "Boolean flag indicating whether to checkpoint at intervals of 1 hour or multiple hours" | ||
help = "Boolean flag indicating whether to checkpoint at multiple-hourly intervals [false (default), true]" | ||
arg_type = Bool | ||
default = false | ||
"--hourly_checkpoint_dt" |
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.
Could you rename this to just checkpoint_dt
and take the same time format as the other time intervals?
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.
Yes and I would like to generalize this, but it requires some changes in TimeManager
since the HourlyCallback
and MonthlyCallback
are hardcoded to take in number of hours/months respectively. It won't be difficult, but I'd rather do it in a follow-up PR to keep this one from getting too large
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 opened an issue for it here #1099
## modify parsed args for fast testing from REPL #hide | ||
if isinteractive() | ||
parsed_args["config_file"] = | ||
isnothing(parsed_args["config_file"]) ? joinpath(pkg_dir, "config/ci_configs/interactive_debug.yml") : | ||
parsed_args["config_file"] | ||
parsed_args["job_id"] = "interactive_debug" | ||
end |
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 we should remove this. We can provide a way debug_config
function if there's such a need
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.
Agreed. We can just add default values for config_file
and job_id
, that will get used if we run interactively without specifying alternatives
function get_coupler_args!(config_dict::Dict) | ||
# Simulation-identifying information; Print `config_dict` if requested | ||
config_dict["print_config_dict"] && @info(config_dict) | ||
mode_name = config_dict["mode_name"] |
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.
Here's a useful pattern for keyword dispatch.
"Type describing what to do with coupling"
abstract type AbstractCouplingMode end
"XYZ"
struct AMIP <: AbstractCouplingMode end
"XYZ"
struct Slabplanet <: AbstractCouplingMode end
# In the function
allowed_modes = Dict(lowercase(string(nameof(t))) => t for t in subtypes(AbstractCouplingMode))
mode_name = lowercase(config_dict["mode_name"])
if haskey(allowed_modes, mode_name)
coupling_mode = allowed_modes[mode_name]()
else
error("$mode_name is not an allowed coupling mode. Allowed modes are: $(keys(allowed_modes))")
end
# Then, you dispatch instead of doing `if-else`
The advantages of this pattern:
- Supports lower/upper cases
- Provides a useful error message
- Automatically extends to new subtypes, including subtypes that a user might define
- Allows us and users to dispatch
- Automatically catches invalid modes
Note, however, that these are probably different experiments, not really CouplingModes
, so the concept is probably ill-posed to begin with.
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 like this idea. Since we're planning to separate the driver based on simulation type soon, I don't want to invest too much time in setting this up. But as we start working on that, we could use this structure
c6817f3
to
d01a9e8
Compare
This commit isolates the argument and configuration file parsing to functions. Since we have one configuration file each for the coupled simulation and for the atmosphere, we end up with the following functions, as well as smaller helper functions. These are located in the new `experiments/ClimaEarth/arg_parsing.jl file`. - `get_coupler_config`, `get_coupler_args`, `get_atmos_args` These functions are called from the driver `run_amip.jl`, where the returned arguments are unpacked. After that point, the config files are not accessed further down in the driver, so we have isolated the config file access to the initial step.
d01a9e8
to
54e6b6b
Compare
Purpose
closes #678
This PR isolates the argument and configuration file parsing to functions. Since we have one configuration file each for the coupled simulation and for the atmosphere, we end up with the following functions, as well as smaller helper functions. These are located in the new
experiments/ClimaEarth/arg_parsing.jl file
.get_coupler_config
,get_coupler_args
,get_atmos_args
These functions are called from the driver
run_amip.jl
, where the returned arguments are unpacked. After that point, the config files are not accessed further down in the driver, so we have isolated the config file access to the initial step.Many files are changed in this PR because argument changes are propagated to many config files. The main files to look at for review are:
run_amip.jl
,arg_parsing.jl
,cli_options.jl
, andclimaatmos.jl
.To-do
argparse
andget_args
functions - want same parameters available from command line and from config filesdt_cpl
to use same format as all other timesrestart_dir
default tonothing
Notes