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

Remove hardcoded parameters, minor improvements for parameters-as-functions #2087

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

nefrathenrici
Copy link
Member

@nefrathenrici nefrathenrici commented Sep 13, 2023

Content

  • Removes entr_coeff detr_coeff and entr_tau from the config, makes them parameters
    • Update edmfx_bomex_box_v2.toml to include the detr_coeff change
  • Adds parameters for previously hardcoded values in the held suarez forcing_tendency, update the parameter struct accordingly
  • Removes SpongeParameters - I think we can keep these in the top-level struct
  • Improves the parameter functions, auto-generate them using @eval
  • Adds some tests to ensure parameters are properly read in. I introduced a bug in the atmos tomls where different parameters had the same alias, leading to parameters not being properly set. This bug has been fixed by ClimaParameters #136, which also includes an error check for unique aliases.

Note: This does not get rid of all hardcoded parameters, only the high-priority ones.

Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! In general, how do we determine if some parameters (e.g. sponge) should be in the top-level struct?

@nefrathenrici
Copy link
Member Author

Looks good, thanks! In general, how do we determine if some parameters (e.g. sponge) should be in the top-level struct?

For now, we can just keep adding parameters to the top level because there aren't too many. If we keep adding parameters, we should probably find better ways to break them up.

@nefrathenrici
Copy link
Member Author

bors r+

@nefrathenrici nefrathenrici linked an issue Sep 19, 2023 that may be closed by this pull request
@bors
Copy link
Contributor

bors bot commented Sep 19, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 83a269d into main Sep 19, 2023
8 checks passed
@bors bors bot deleted the ne/hs_params branch September 19, 2023 22:46
@nefrathenrici nefrathenrici mentioned this pull request Sep 21, 2023
4 tasks
bors bot added a commit that referenced this pull request Sep 27, 2023
2149: Move parameter logging to `get_integrator` r=nefrathenrici a=nefrathenrici

From `@Sbozzolo's` comment on #2087


Co-authored-by: nefrathenrici <nat.henrici@gmail.com>
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.

A few hardcoded numbers
5 participants