Move parameters out 1M options types into Microphysics1MParams - #767
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.
| 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.
There was a problem hiding this comment.
Agreed processes / process_params is clearer, I can make the change.
There was a problem hiding this comment.
Actually, it may be a larger effort to get everything named consistently. I think it should be done in a later PR.
There was a problem hiding this comment.
I agree, let's make that suggestion a separate PR. But it'd also be good to hear from @trontrytel if she has thoughts about this bike shed's color:)
There was a problem hiding this comment.
Yes on consistency. No preferences on the color of the bikeshed
|
|
||
| # Fields | ||
| $(DocStringExtensions.FIELDS) | ||
| Parameters (`τ_relax`) are stored in `process_params.cloud_liquid_formation`. |
There was a problem hiding this comment.
many docstrings mention where some parameter is stored. could this be made a bit more explicit by mentioning the struct storing them, e.g.,
stored in `process_params.cloud_liquid_formation` in [`Microphysics1MParams`](@ref)or similar
| # Fields | ||
| - `options::OPT`: Microphysics1MOptions — process configuration (carries process-specific parameters) | ||
| - `options::OPT`: Microphysics1MOptions — process selection | ||
| - `process_params::PPR`: parameter data for each selected process, mirroring `options` (`nothing` when a process is disabled with `nothing` or needs no parameters) |
| @inline function conv_q_lcl_to_q_rai(::CMP.Kessler1M, mp, tps, micro, thermo) | ||
| q_lcl = micro.q_lcl | ||
| (; τ, q_threshold, k) = opt.acnv1M | ||
| (; τ, q_threshold, k) = mp.process_params.rain_autoconversion |
There was a problem hiding this comment.
Sorry, I'm a little late into this. I thought we wanted the process_params struct to be flat? Since we are no longer dispatching, there is no need for the additional layer of .rain_autoconversion ?
There was a problem hiding this comment.
The per-process key is still needed, but not for dispatch. process_params holds the params for every enabled process at once and some of these parameters have the same name. To flatten this structure, every parameter would need to have a unique name.
There was a problem hiding this comment.
Yes. I thought that was the idea? I don't think there are that many with doubled names tbh. I thought it would be easier on the compiler to have a flat struct here, rather than more nesting?
3 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.