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.

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

specify dt in benchmark configs #1076

Merged
merged 1 commit into from
Nov 13, 2024
Merged

specify dt in benchmark configs #1076

merged 1 commit into from
Nov 13, 2024

Conversation

juliasloan25
Copy link
Member

Purpose

The benchmark runs failed this weekend because dt was not specified correctly (see error here).

The benchmark AMIP config files (e.g. config/benchmark_configs/amip_diagedmf.yml) were previously relying on dt being specified in the atmos config files they use (e.g. config/benchmark_configs/climaatmos_diagedmf.yml), rather than specifying dt explicitly in the AMIP configs. When the configs were merged in get_atmos_config_dict, the atmos config's dt was inherited by the coupled config file (here). After #1059, we overwrite the dt entry in the atmos config with the entry in the coupler config (here). This requires that the coupler config does have an entry for dt (or all component dts), so we can no longer rely on the atmos dt.

This is a change in behavior, but the previous behavior was not necessarily desired. It seems reasonable to me that the config file requires that dt is specified there, rather than inherited from an atmosphere config, so I think we can keep the current setup and just add dt entries to the coupled configs that didn't have it before.

To Do

  • Add a dt entry to AMIP benchmark configs
  • Verify that all configs specify dt or component model dts

Copy link
Contributor

@imreddyTeja imreddyTeja left a comment

Choose a reason for hiding this comment

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

Seems good to me. In general, why do we merge atmos_config_dict into config_dict? The merge adds a lot of fields to config_dict, and it seems like only one of the fields that gets added intoconfig_dict is accessed later on.

@juliasloan25
Copy link
Member Author

Seems good to me. In general, why do we merge atmos_config_dict into config_dict? The merge adds a lot of fields to config_dict, and it seems like only one of the fields that gets added intoconfig_dict is accessed later on.

That's a good question! I'm not really sure why - my guess is that during initial coupler development we relied heavily on simulation parameters defined in atmos config files, rather than defining our own. From a design perspective, it would be better to isolate anything we need to set up the atmos simulation in atmos_config_dict, and anything we need to set up/run the coupled simulation in config_dict, and remove the merge. We should clean this up as we isolate the argument parsing in the driver.

Which is the one field that gets added into config_dict and accessed later on?

@imreddyTeja
Copy link
Contributor

We should clean this up as we isolate the argument parsing in the driver.

I didn't look into it super carefully, but I only saw dt_rad being added and then later accessed.

@juliasloan25 juliasloan25 merged commit 2a9f7b7 into main Nov 13, 2024
9 checks passed
@juliasloan25 juliasloan25 deleted the js/benchmark-config branch November 13, 2024 00:57
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.

2 participants