Move parameters out 1M options types into Microphysics1MParams #767
Move parameters out 1M options types into Microphysics1MParams #767nefrathenrici wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #767 +/- ##
==========================================
- Coverage 92.92% 92.86% -0.06%
==========================================
Files 56 56
Lines 2896 2889 -7
==========================================
- Hits 2691 2683 -8
- Misses 205 206 +1
🚀 New features to boost your workflow:
|
haakon-e
left a comment
haakon-e
left a comment
There was a problem hiding this comment.
Looks good. some minor inline comments attached.
The biggest piece that it would be useful if this PR documented (at least in the PR description) is why this change is desired. Some motivation is provided in #766 which appear to primarily point to ergonomics:
[currently,] you can't choose a process without a full parameter set, complicating the atmosphere model interface downstream.
If I understood some offline conversations correctly, this change could also be beneficial for performance. It would be useful to spell that out (and test it) in this PR too.
| year = {2016} | ||
| } | ||
|
|
||
| @Article{KnopfAlpert2013, |
There was a problem hiding this comment.
why remove this? is it unused?
| rain_snow_accretion = process_params_for(o.rain_snow_accretion, td), | ||
| ) | ||
|
|
||
| # ═══════════════════════════════════════════════════════════════════ |
There was a problem hiding this comment.
I personally think it's fine to skip deprecation.
| options::OPT | ||
| process_params::PPR |
There was a problem hiding this comment.
small nit: I think it would be more intuitive if the naming between options and process_params was uniform. That is, name both either:
options,option_paramsprocesses,process_params
I have a slight preference towards the latter, but bike shedding like this can also wait until another day.
|
|
||
| # Fields | ||
| - `options::OPT`: Microphysics1MOptions — process configuration (carries process-specific parameters) | ||
| - `options::OPT`: Microphysics1MOptions — process selection (no parameters) |
There was a problem hiding this comment.
| - `options::OPT`: Microphysics1MOptions — process selection (no parameters) | |
| - `options::OPT`: Microphysics1MOptions — process selection |
| # Fields | ||
| - `options::OPT`: Microphysics1MOptions — process configuration (carries process-specific parameters) | ||
| - `options::OPT`: Microphysics1MOptions — process selection (no parameters) | ||
| - `process_params::PPR`: per-process parameter data matching `options` (or `nothing` for parameter-free/disabled processes) |
There was a problem hiding this comment.
what does
nothingfor [...] disabled processes
mean?
| Process selection (which variant of each process runs) lives in `options`. The | ||
| parameter values each selected variant needs (relaxation timescales, | ||
| autoconversion thresholds, collision efficiencies) live in `process_params`, | ||
| whose fields mirror `options`. Shared parameters (particle size distributions, | ||
| air properties, terminal velocities) are stored directly. |
There was a problem hiding this comment.
Perhaps an example would help make this a bit clearer? The language is a bit dense and abstract right now, I think, for something that shouldn't be that hard to grasp.
| _conv_q_vap_to_q_lcl_const( | ||
| mp.process_params.cloud_liquid_formation.τ_relax, tps, micro, thermo) | ||
|
|
||
| # Kernel for `conv_q_vap_to_q_lcl`with a constant relaxation timescale `τ`. | ||
| @inline function _conv_q_vap_to_q_lcl_const(τ, tps::TDI.PS, micro, thermo) |
There was a problem hiding this comment.
why do we have inner _-functions for this and _conv_q_vap_to_q_icl_const?
2 participants
Changes
process_paramsinMicrophysics1MParams.process_params_for(option, toml_dict)reads the parameter values for each selected process.mp.process_params.<slot>instead of off the option struct. Dropped the now-unused opt argument names._conv_q_vap_to_q_lcl_const,_conv_q_vap_to_q_icl_const) so the 2M scheme can reuse them. Temperature-dependent ice stays inlined.Microphysics1MOptions.Some benchmark tests fail on the 1.12 runner, I would like to increase the time limit since there is no type-stability change.