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 case-specific overrides from parameter creation #2017

Merged
merged 1 commit into from
Sep 18, 2023
Merged

Conversation

nefrathenrici
Copy link
Member

@nefrathenrici nefrathenrici commented Aug 25, 2023

Content

  • Removes case-specific parameter overrides. This changes simulation results, which have been checked by @szy21 (including longruns). Relevant builds: default pipeline, longruns
  • The exception to the above is τ_precip, which is still set to dt in all cases. This is the same behavior as before, and but can now be switched off by setting the new config flag override_τ_precip = false
  • Adds a function create_parameter_struct which takes in a parameter struct type, any nested parameter structs, and constructs the parameters. This cleans up a lot of boilerplate from create_parameter_set
  • Removes unused parameters: ug, vg, f, and Cd. It was hard to grep for usage of f but all of my local tests work.
  • Sets f_plane_coriolis_frequencyvia CLIMAParameters instead of a hardcoded value. The default value is unchanged

@trontrytel trontrytel linked an issue Aug 25, 2023 that may be closed by this pull request
@nefrathenrici nefrathenrici mentioned this pull request Aug 28, 2023
4 tasks
@nefrathenrici nefrathenrici force-pushed the ne/overrides branch 2 times, most recently from d1d50ee to 4946cfb Compare September 7, 2023 16:07
@nefrathenrici nefrathenrici force-pushed the ne/overrides branch 2 times, most recently from 1aa52fc to 86210f9 Compare September 11, 2023 20:45
@Sbozzolo
Copy link
Member

Where/how is τ_precip used? I don't see any reference to it in ClimaAtmos except in setting the parameter

@nefrathenrici
Copy link
Member Author

Where/how is τ_precip used? I don't see any reference to it in ClimaAtmos except in setting the parameter

@Sbozzolo It's used in CloudMicrophysics and is used in periodically removing precipitation in the 0Moment microphysics scheme. It's kind of an exception since it is always set to dt. There is likely a better way of passing it through to CloudMicrophysics but I just want to get rid of the case-specific overrides for now!

@Sbozzolo
Copy link
Member

Sbozzolo commented Sep 12, 2023

Where/how is τ_precip used? I don't see any reference to it in ClimaAtmos except in setting the parameter

@Sbozzolo It's used in CloudMicrophysics and is used in periodically removing precipitation in the 0Moment microphysics scheme. It's kind of an exception since it is always set to dt. There is likely a better way of passing it through to CloudMicrophysics but I just want to get rid of the case-specific overrides for now!

Thanks! How is it passed to CloudMicrophysics? If it is always set to dt, maybe we can implement it as a callback that executes at the end of each integration step?

(My interest in this stems from the fact that I am studying how to remove explicit references to parsed_args past the configuration stage.)

@szy21
Copy link
Member

szy21 commented Sep 12, 2023

It would be better to just ask the user to specify both τ_precip and dt. They don't have to be the same.

@nefrathenrici
Copy link
Member Author

@Sbozzolo I'm not sure how it would work as a callback, since it needs to be passed in as a parameter to CloudMicrophysics, but I'm open to changing how it's set.

@nefrathenrici nefrathenrici linked an issue Sep 12, 2023 that may be closed by this pull request
Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

Changes overall LGTM

@nefrathenrici nefrathenrici force-pushed the ne/overrides branch 2 times, most recently from 01d5d44 to 000bf30 Compare September 18, 2023 17:29
@nefrathenrici
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 18, 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 cb44933 into main Sep 18, 2023
8 checks passed
@bors bors bot deleted the ne/overrides branch September 18, 2023 18:42
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.

Get rid of ClimaAtmos case-specific parameter overrides. Unify parameter sets
4 participants