Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical compile and configuration migration issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors distribution configuration so SD concentration and multiplicity are specified per distribution, updating initialization, sources, bindings, tests, and documentation.
Changes:
- Introduces tuple-keyed per-distribution settings.
- Updates source and relaxation handling.
- Migrates Python/C++ tests and API documentation.
File summaries
| File | Summary | Review note |
|---|---|---|
tests/python/unit/uniform_init.py |
Migrates distribution configuration. | — |
tests/python/unit/terminal_velocities.py |
Migrates SD count configuration. | — |
tests/python/unit/sstp_cond.py |
Migrates distribution keys. | — |
tests/python/unit/source.py |
Updates source keys and constant-multiplicity coverage. | — |
tests/python/unit/segfault_20150216.py |
Migrates distribution configuration. | — |
tests/python/unit/SD_removal.py |
Migrates distribution configuration. | — |
tests/python/unit/relax.py |
Updates relaxation setup. | — |
tests/python/unit/multiple_kappas.py |
Tests per-distribution counts and multiplicities. | — |
tests/python/unit/lgrngn_turb_adve.py |
Migrates distribution configuration. | — |
tests/python/unit/lgrngn_subsidence.py |
Migrates distribution configuration. | — |
tests/python/unit/lgrngn_adve.py |
Migrates distribution configuration. | — |
tests/python/unit/ice_SD.py |
Migrates distribution configuration. | — |
tests/python/unit/diag_incloud_time.py |
Migrates per-distribution counts. | — |
tests/python/unit/col_kernels.py |
Migrates distribution configuration. | — |
tests/python/unit/chem_coal.py |
Migrates distribution configuration. | — |
tests/python/unit/api_lgrngn.py |
Updates API scenarios. | — |
tests/python/unit/adve_scheme.py |
Migrates distribution configuration. | — |
tests/python/physics/test_coal.py |
Migrates distribution configuration. | — |
tests/python/physics/puddle.py |
Migrates distribution configuration. | — |
tests/python/physics/lgrngn_cond.py |
Migrates distribution configuration. | — |
tests/python/physics/lgrngn_cond_substepping.py |
Migrates distribution configuration. | — |
tests/python/physics/coalescence_onishi_hall.py |
Migrates coalescence setup. | critical (3 votes): Use the required four-field distribution key. |
tests/python/physics/coalescence_hall_davis_no_waals.py |
Migrates distribution configuration. | — |
tests/python/physics/coalescence_golovin.py |
Migrates constant-multiplicity configuration. | — |
tests/mpi/mpi_adve_test.cpp |
Migrates C++ distribution configuration. | — |
src/impl/sources_and_relaxation_of_SDs/particles_impl_src_dry_sizes.ipp |
Handles tuple-based source size settings. | — |
src/impl/sources_and_relaxation_of_SDs/particles_impl_src_dry_distros.ipp |
Updates source distribution lookup. | — |
src/impl/sources_and_relaxation_of_SDs/particles_impl_src_dry_distros_simple.ipp |
Adds per-source multiplicity selection. | — |
src/impl/sources_and_relaxation_of_SDs/particles_impl_src_dry_distros_matching.ipp |
Updates matching-source configuration. | moderate (2 votes): Validate positive sd_conc and zero sd_const_multi. |
src/impl/sources_and_relaxation_of_SDs/particles_impl_rlx_dry_distros.ipp |
Updates relaxation initialization. | — |
src/impl/particles_impl.ipp |
Updates initialization state and mode detection. | moderate (1 vote): Handle mixed multiplicities conservatively when computing pure_const_multi. |
src/impl/initialization/particles_impl_init_SD_with_sizes.ipp |
Reads tuple-based size keys. | — |
src/impl/initialization/particles_impl_init_SD_with_distros.ipp |
Initializes each distribution independently. | — |
src/impl/initialization/particles_impl_init_SD_with_distros_sd_conc.ipp |
Uses per-distribution SD counts. | — |
src/impl/initialization/particles_impl_init_SD_with_distros_const_multi.ipp |
Uses per-distribution multiplicities. | — |
src/impl/initialization/particles_impl_init_sanity_check.ipp |
Validates distribution-specific settings. | moderate (1 vote): Reject distributions where both values are zero. |
src/impl/initialization/particles_impl_init_count_num.ipp |
Adds source constant-multiplicity counting. | — |
include/libcloudph++/lgrngn/opts_init.hpp |
Removes global SD configuration fields. | — |
include/libcloudph++/lgrngn/distro_t.hpp |
Defines tuple-based distribution types. | critical (3 votes): Migrate the kinematic_2D model before removing old symbols. moderate (1 vote): Include <tuple> explicitly. |
docs/USER_OPTIONS.md |
Documents the new configuration format. | nit (2 votes): Document dry_distros[].sd_conc. |
docs/API.md |
Updates API references. | nit (3 votes): Update the stale Lagrangian example to the new tuple-keyed API. |
bindings/python/lib.cpp |
Removes obsolete Python option bindings. | — |
bindings/python/lgrngn.hpp |
Parses tuple-based Python keys. | — |
Review details
Suppressed comments (3)
include/libcloudph++/lgrngn/distro_t.hpp:14
- This public header now names
std::tuplein all distribution aliases but does not include<tuple>(and the repository has no direct tuple include in this header's include chain). Consumers that includedistro_t.hppwithout an unrelated transitive tuple include can fail to compile; add the standard header explicitly.
std::tuple<real_t, real_t, unsigned long long, unsigned long long>, // kappa, soluble_fraction, sd_conc, sd_const_multi
src/impl/initialization/particles_impl_init_sanity_check.ipp:87
- The new per-entry check rejects only the both-positive case. If one valid distribution is present, a second distribution with
(sd_conc, sd_const_multi) == (0, 0)passes this validation and is then silently skipped byinit_SD_with_distros()because neither initialization branch runs. Reject the neither-positive case for each distribution as well.
if(sd_conc > 0 && sd_const_multi > 0)
throw std::runtime_error("libcloudph++: specify either sd_conc or sd_const_multi for each dry distribution, not both");
src/impl/particles_impl.ipp:386
- The new key allows each distribution to use a different constant multiplicity (as in
tests/python/unit/multiple_kappas.py:88), but this predicate only checks that no distribution usessd_concand that at least one usessd_const_multi. It therefore marks mixed multiplicities aspure_const_multi, sending coalescence/recycling through the homogeneous-constant path even though the run is mixed. Compute this flag only when all active SDs share one multiplicity, or conservatively disable it for mixed values.
pure_const_multi (std::none_of(_opts_init.dry_distros.cbegin(), _opts_init.dry_distros.cend(), [](const auto &distro) { return std::get<2>(distro.first) > 0; }) && (std::any_of(_opts_init.dry_distros.cbegin(), _opts_init.dry_distros.cend(), [](const auto &distro) { return std::get<3>(distro.first) > 0; }) || _opts_init.dry_sizes.size() > 0)), // coal prob can be greater than one only in sd_conc simulations
- Files reviewed: 43/43 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| template<typename real_t> | ||
| using dry_distros_t = std::map< | ||
| kappa_soluble_fraction_t<real_t>, // (kappa, soluble_fraction); dry_distros defines total dry radius | ||
| std::tuple<real_t, real_t, unsigned long long, unsigned long long>, // kappa, soluble_fraction, sd_conc, sd_const_multi |
| diss_rate = epsilon * np.ones((1,)) | ||
|
|
||
| opts_init.dry_distros = {0.:expvolumelnr} | ||
| opts_init.dry_distros = {(0., 1024, 0):expvolumelnr} |
Comment on lines
+55
to
+56
| if(get<3>(p_sdd->first) > 0) | ||
| throw std::runtime_error("libcloudph++: constant multiplicity source distributions are only supported with src_t::simple"); |
| See [USER_OPTIONS.md](USER_OPTIONS.md#lagrangian-lgrngn-initialization-options) for complete documentation of initialization options including: | ||
| - Domain configuration (nx, ny, nz, dx, dy, dz) | ||
| - Super-droplet configuration (sd_conc, sd_conc_mean) | ||
| - Super-droplet configuration (per-distribution sd_conc, sd_const_multi) |
| |--------|------|---------|-------------| | ||
| | `sd_conc` | `unsigned long long` | `0` | Number of super-droplets per cell | | ||
| | `sd_const_multi` | `unsigned long long` | `0` | Alternative to `sd_conc`: constant multiplicity for all SDs | | ||
| | `dry_distros[].sd_const_multi` | `unsigned long long` | `0` | Constant multiplicity for the distribution; cannot be used with that distribution's `sd_conc` | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.