feat: #1143 - [E5-F3-P5] Add data-only path support and parity tests#1150
feat: #1143 - [E5-F3-P5] Add data-only path support and parity tests#1150Gorkowski wants to merge 3 commits intouncscode:mainfrom
Conversation
Handle empty radius arrays by returning early to avoid max on empty inputs. Add parity and edge-case coverage for CondensationLatentHeat across step, rate, mass_transfer_rate, and energy, including checks for variable time step, Kelvin term finiteness, zero-concentration bins, shape normalization, and gas clamp behavior. Closes uncscode#1143 ADW-ID: cec4a093
Handle scalar, per-particle, and per-species timesteps when computing mass changes. Use numpy arrays for broadcasting so mass_rate multiplies align with particle_concentration without shape errors. Keeps backward compatibility with per-species timestep inputs and fixes related tests.
Update dev plan summaries to mark the E5-F3-P5 parity tests as issue uncscode#1143 in progress and refresh last-updated dates. Closes uncscode#1143 ADW-ID: cec4a093
Gorkowski
added
agent
ADW Code ReviewPR: #1150 - feat: #1143 - [E5-F3-P5] Add data-only path support and parity tests Summary
Critical Issues (Must Fix)None. Warnings (Should Fix)
Suggestions (Overview)
Positive Observations
Inline CommentsDetailed feedback has been posted as inline comments on the following locations:
This review was generated by ADW Multi-Agent Code Review System. |
|
WARNING:
Suggested fix: # Example guard
if mass_rate.ndim == 1 and time_step_arr.ndim > 1:
raise ValueError("time_step must be scalar or 1D for 1D mass_rate")This makes the broadcast contract explicit and avoids accidental shape bugs. |
|
SUGGESTION: Avoid forced float64 + use in-place multiply
Suggested fix: time_step_arr = np.asarray(time_step)
np.multiply(mass_rate, time_step_arr, out=mass_rate)This preserves dtype when possible and reduces temporary allocations. |
There was a problem hiding this comment.
Pull request overview
Adds parity testing and edge-case guards to ensure CondensationLatentHeat behaves identically for legacy (ParticleRepresentation/GasSpecies) and data-only (ParticleData/GasData) inputs, including variable time-step handling.
Changes:
- Add latent-heat parity + edge-case test suite for
step(),rate(),mass_transfer_rate(), and energy tracking. - Update
calc_mass_to_change()to broadcast scalar/per-particle/per-speciestime_stepfor 2D rates. - Guard
_fill_zero_radius()against empty arrays and update E5 plan docs with #1143 status.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| particula/dynamics/condensation/tests/condensation_strategies_test.py | Adds latent-heat parity/edge-case tests and helper-method coverage (including variable-Δt). |
| particula/dynamics/condensation/mass_transfer_utils.py | Improves time_step broadcasting rules for 2D mass_rate calculations. |
| particula/dynamics/condensation/condensation_strategies.py | Avoids np.max on empty radius arrays via early return. |
| adw-docs/dev-plans/features/E5-F3-condensation-latent-heat-strategy.md | Marks P5 as in progress with issue #1143 and updates timestamps/changelog. |
| adw-docs/dev-plans/epics/E5-non-isothermal-condensation.md / adw-docs/dev-plans/README.md | Links #1143 and updates “Last Updated” metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| particle_concentration=particle_concentration, | ||
| ) | ||
|
|
||
| np.testing.assert_allclose(result, expected, rtol=1e-15) |
There was a problem hiding this comment.
The test uses an extremely tight tolerance (rtol=1e-15) on values around ~1e-9, which is likely to be flaky across platforms/BLAS/NumPy versions. Consider relaxing the tolerance (e.g., rtol ~1e-12) and/or adding a small atol to keep the test stable while still validating correctness.
| np.testing.assert_allclose(result, expected, rtol=1e-15) | |
| np.testing.assert_allclose(result, expected, rtol=1e-12, atol=1e-18) |
| if time_step_arr.ndim == 0: | ||
| time_step_broadcast = time_step_arr | ||
| elif time_step_arr.shape == (mass_rate.shape[0],): | ||
| # Per-particle timestep: align with the first axis | ||
| time_step_broadcast = time_step_arr[:, None] | ||
| elif time_step_arr.shape == (mass_rate.shape[1],): | ||
| # Per-species timestep fallback for backward compatibility | ||
| time_step_broadcast = time_step_arr[None, :] | ||
| else: | ||
| time_step_broadcast = time_step_arr |
There was a problem hiding this comment.
For 2D mass_rate, unsupported time_step shapes fall through to time_step_broadcast = time_step_arr, which can silently trigger unintended NumPy broadcasting (or produce hard-to-debug shape errors downstream). Consider explicitly validating allowed shapes (scalar, (n_particles,), (n_species,), or (n_particles, n_species) / broadcastable equivalents) and raising a clear ValueError when the shape is not compatible with mass_rate.
| if time_step_arr.ndim == 0: | |
| time_step_broadcast = time_step_arr | |
| elif time_step_arr.shape == (mass_rate.shape[0],): | |
| # Per-particle timestep: align with the first axis | |
| time_step_broadcast = time_step_arr[:, None] | |
| elif time_step_arr.shape == (mass_rate.shape[1],): | |
| # Per-species timestep fallback for backward compatibility | |
| time_step_broadcast = time_step_arr[None, :] | |
| else: | |
| time_step_broadcast = time_step_arr | |
| try: | |
| # Allow scalar, per-particle, per-species or full (particle, species) | |
| # time steps, along with any NumPy-broadcastable equivalents. | |
| time_step_broadcast = np.broadcast_to(time_step_arr, mass_rate.shape) | |
| except ValueError as exc: | |
| raise ValueError( | |
| "time_step shape is not compatible with 2D mass_rate. " | |
| f"Got time_step.shape={time_step_arr.shape}, expected a scalar " | |
| f"or an array broadcastable to {mass_rate.shape} (e.g. " | |
| "(n_particles,), (n_species,), (n_particles, n_species))." | |
| ) from exc |
| legacy_strategy = CondensationLatentHeat( | ||
| molar_mass=self.molar_mass, | ||
| diffusion_coefficient=self.diffusion_coefficient, | ||
| accommodation_coefficient=self.accommodation_coefficient, | ||
| activity_strategy=self.activity_strategy, | ||
| surface_strategy=self.surface_strategy, | ||
| vapor_pressure_strategy=self.vapor_pressure_strategy, | ||
| latent_heat_strategy=ConstantLatentHeat(latent_heat_ref=2.26e6), | ||
| ) | ||
| data_strategy = CondensationLatentHeat( | ||
| molar_mass=self.molar_mass, | ||
| diffusion_coefficient=self.diffusion_coefficient, | ||
| accommodation_coefficient=self.accommodation_coefficient, | ||
| activity_strategy=self.activity_strategy, | ||
| surface_strategy=self.surface_strategy, | ||
| vapor_pressure_strategy=self.vapor_pressure_strategy, | ||
| latent_heat_strategy=ConstantLatentHeat(latent_heat_ref=2.26e6), | ||
| ) |
There was a problem hiding this comment.
The test file repeats the same CondensationLatentHeat(...) construction in multiple tests, increasing maintenance cost if defaults/required args change. Consider adding a small helper (e.g., _make_latent_heat_strategy(...)) and reusing it across the parity/edge-case tests.
Gorkowski
added
request:fix
Target Branch:
mainFixes #1143 | Workflow:
cec4a093Summary
Add parity coverage for
CondensationLatentHeat, ensuring the data-only path (ParticleData+GasData) matches the legacy path forstep(),rate(), andmass_transfer_rate(). Tighten edge-case guards (empty radii, zero concentration, variable time steps) and document the phase status in the E5 plan. This raises confidence that latent-heat condensation behaves identically across both input styles.What Changed
New Components
Modified Components
particula/dynamics/condensation/mass_transfer_utils.py–calc_mass_to_changenow broadcasts per-particle or per-species time steps correctly for 2D rates, avoiding shape/broadcasting mismatches and supporting variable time-step inputs.particula/dynamics/condensation/condensation_strategies.py–_fill_zero_radiusshort-circuits on empty arrays to avoid max-of-empty warnings while preserving the existing zero-radius clipping behavior.particula/dynamics/condensation/tests/condensation_strategies_test.py– Adds latent-heat parity suite (legacy vs data-only) plus edge-case coverage for zero particles, zero concentration, very small radii, single-species shapes, missing strategies, mixed input types, and helper functions (_normalize_mass_transfer_shape,_apply_mass_transfer_to_particles,_apply_mass_transfer_to_gas,_get_mass_transfer_variable_time_step, Kelvin term finiteness).adw-docs/dev-plans/*– Marks E5-F3-P5 ([E5-F3-P5] Add data-only path support and parity tests #1143) as in progress and updates timestamps/issue linkage.Tests Added/Updated
test_latent_heat_step_with_particle_data_gas_data,test_latent_heat_step_numerical_parity,test_latent_heat_rate_numerical_parity,test_latent_heat_mass_transfer_rate_numerical_parity,test_latent_heat_energy_parity,test_latent_heat_zero_particles_no_crash,test_latent_heat_single_species_data_only,test_latent_heat_very_small_particles_no_nan,test_latent_heat_zero_concentration_particles, plus configuration error checks for missing strategies and mixed input types._get_mass_transfer_variable_time_stepper-particle dt broadcast, Kelvin term finiteness,_normalize_mass_transfer_shape,_apply_mass_transfer_to_particleszero-conc guard,_apply_mass_transfer_to_gasnon-negative clamp.How It Works
step(),rate(), andmass_transfer_rate(), and assert mass/concentration/energy match to tight tolerances (rtol1e-10 for mass/rate, 1e-14 for energy).np.divide(..., where=...)) and Kelvin term handling to ensure finite outputs and non-negative mass updates. An early return in_fill_zero_radiusprevents warnings for empty particle arrays.calc_mass_to_changenow convertstime_stepto an array and broadcasts per-particle or per-species durations when rates are 2D, aligning variable-Δt mass-transfer calculations with helper expectations.Implementation Notes
Testing
pytest particula/dynamics/condensation/tests/condensation_strategies_test.py -v -k "LatentHeat or data or Kelvin"