feat: #1142 - [E5-F3-P4] Add discrete and continuous distribution support with tests#1149
Conversation
Normalize first-order mass transport inputs for binned pressure arrays and add discrete/PDF latent heat tests for mass conservation, isothermal parity, energy tracking, and shapes. Closes uncscode#1142 ADW-ID: d6b71842
Successfully fixed: - Clarified continuous-bin isothermal parity test docstring to state latent-heat-zero parity expectation Still failing: - None Closes uncscode#1142 ADW-ID: d6b71842
Record the P4 issue assignment and update timestamps in the epic and feature plans to reflect the in-progress status. Closes uncscode#1142 ADW-ID: d6b71842
Gorkowski
added
agent
ADW Code ReviewPR: #1149 - feat: #1142 - [E5-F3-P4] Add discrete and continuous distribution support with tests Summary
Critical Issues (Must Fix)1. Per-particle Python loop in staggered Gauss–Seidel path; vectorize/batch.
# Example: batch vectorize with NumPy/array operations
# (Implement appropriate batching for the solver state updates)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. |
|
CRITICAL: Per-particle loop in staggered Gauss–Seidel path The current per-particle Python loop will not scale for large particle counts and will dominate runtime. Suggested fix: # Vectorize/batch updates across particles
# e.g., compute per-particle terms as arrays and update in bulkThis reduces Python overhead and improves throughput on large simulations. |
|
WARNING: Non-finite Kelvin term can raise in
Suggested fix: if not np.isfinite(kelvin_term):
# handle/clamp or raise with a clearer messageThis keeps error handling consistent with the other non-finite guards. |
|
WARNING: Numerical noise can produce small negatives; raising unconditionally can be overly strict. Suggested fix: if np.any(conc < -tol):
raise ValueError(...)
conc = np.maximum(conc, 0.0)This preserves strictness while tolerating floating-point noise. |
|
WARNING:
Suggested fix: @validate_inputs({"time_step": "positive"})
# or explicit checksThis prevents silent propagation of invalid time steps. |
|
WARNING: The override signature differs from the base Suggested fix: # Align signature with base class or update interface + usagesThis keeps polymorphism safe and avoids unexpected runtime errors. |
There was a problem hiding this comment.
Pull request overview
Adds latent-heat condensation support validation for binned (PMF) and continuous (PDF) particle distributions, with test coverage and small shape/broadcasting adjustments to keep mass-transfer calculations consistent for 2D (n_bins, n_species) inputs.
Changes:
- Add binned PMF/PDF fixtures plus mass conservation, isothermal parity, energy bookkeeping, shape, and error-path tests for
CondensationLatentHeat. - Normalize
first_order_mass_transportshape inmass_transfer_rate()whenpressure_deltais 2D to ensure correct NumPy broadcasting for binned/multi-species cases. - Update E5 latent-heat epic/feature planning docs to reference issue #1142.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
particula/dynamics/condensation/condensation_strategies.py |
Ensures first_order_mass_transport broadcasts correctly against 2D pressure_delta in both isothermal and latent-heat strategies. |
particula/dynamics/condensation/tests/condensation_strategies_test.py |
Adds PMF/PDF binned fixtures and tests for latent-heat mass conservation, parity, energy diagnostics, shapes, and input validation. |
adw-docs/dev-plans/features/E5-F3-condensation-latent-heat-strategy.md |
Links phase P4 to issue #1142 and updates plan status/date. |
adw-docs/dev-plans/epics/E5-non-isothermal-condensation.md |
Links E5-F3-P4 to issue #1142 and updates last-updated/date log. |
adw-docs/dev-plans/README.md |
Adds #1142 reference to the E5-F3 status line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_step_rejects_nonpositive_concentration(self): | ||
| """Binned step rejects nonpositive concentrations.""" | ||
| particle, particle_data, gas_species, gas_data = ( | ||
| self._build_binned_representation("pmf", 10) | ||
| ) | ||
| particle_data.concentration[0][0] = -1.0 | ||
| cond = CondensationLatentHeat( |
There was a problem hiding this comment.
test_step_rejects_nonpositive_concentration (and its docstring) says “nonpositive”, but the production check it exercises rejects only negative concentrations (concentration < 0). Since zero concentration bins are valid/expected for binned distributions, consider renaming this test (and/or updating the docstring) to “negative” to avoid implying zeros should raise.
| first_order_mass_transport = np.asarray( | ||
| first_order_mass_transport, dtype=np.float64 | ||
| ) | ||
| if pressure_delta.ndim == 2: | ||
| if first_order_mass_transport.ndim == 0: | ||
| first_order_mass_transport = np.full( | ||
| (pressure_delta.shape[0], 1), | ||
| float(first_order_mass_transport), | ||
| dtype=np.float64, | ||
| ) | ||
| elif first_order_mass_transport.ndim == 1: | ||
| first_order_mass_transport = first_order_mass_transport[:, None] | ||
|
|
There was a problem hiding this comment.
The first_order_mass_transport reshaping/broadcasting logic added here is duplicated verbatim in both CondensationIsothermal.mass_transfer_rate and CondensationLatentHeat.mass_transfer_rate. To avoid future divergence, consider extracting this into a small shared helper (e.g., a private function on the base class) and unit-test it once.
There was a problem hiding this comment.
fix tests,
FAILED particula/dynamics/condensation/tests/condensation_strategies_test.py::TestCondensationLatentHeat::test_step_continuous_isothermal_parity - AssertionError:
Not equal to tolerance rtol=1e-15, atol=0
Mismatched elements: 1 / 3 (33.3%)
Mismatch at index:
[0]: 0.0 (ACTUAL), -3.469446951953614e-18 (DESIRED)
Max absolute difference among violations: 3.46944695e-18
Max relative difference among violations: 1.
ACTUAL: array([0.000000e+00, 8.583921e-24, 4.766954e-24])
DESIRED: array([-3.469447e-18, 8.583921e-24, 4.766954e-24])
===== 1 failed, 1434 passed, 28 skipped, 34 deselected in 72.64s (0:01:12) =====
Gorkowski
added
request:fix
🤖 Agent Developer Workflow
Current Status
Workflow Plan
📋 Recent Activity🔍 Archived UpdatesThis comment is automatically updated as the workflow progresses |
Successfully fixed: - Added time_step validation and tests for isothermal and latent heat steps. - Guarded non-finite Kelvin terms in vapor pressure surface handling. - Clamped small negative concentrations in norm_conc with tolerance. - Deduplicated transport normalization and added coverage for 2D shapes. - Improved staggered batch updates to reduce per-particle overhead. - Adjusted parity test tolerances and renamed negative concentration test. - Updated condensation planning docs to reflect current issue status. Still failing (if any): - No remaining failures. Notes: - Wrapped long lines in condensation_strategies.py to satisfy ruff E501. Closes uncscode#1149 ADW-ID: 86d29772
Handle vectorized time steps in staggered condensation by routing array inputs through shared mass-transfer limiting utilities. This prevents test failures when dt varies per particle and keeps limits consistent across scalar and vectorized paths.
Fix SummaryAddressed actionable review feedback for PR #1149, including condensation strategy refactors, validation updates, and targeted test adjustments. Changes Made
Review Comments Addressed
Files Changed3 files changed, 855 insertions(+), 31 deletions(-) Automated fix by ADW workflow |
Successfully fixed: - Added time_step validation and tests for isothermal and latent heat steps. - Guarded non-finite Kelvin terms in vapor pressure surface handling. - Clamped small negative concentrations in norm_conc with tolerance. - Deduplicated transport normalization and added coverage for 2D shapes. - Improved staggered batch updates to reduce per-particle overhead. - Adjusted parity test tolerances and renamed negative concentration test. - Updated condensation planning docs to reflect current issue status. Still failing (if any): - No remaining failures. Notes: - Wrapped long lines in condensation_strategies.py to satisfy ruff E501. Closes #1149 ADW-ID: 86d29772
Target Branch:
mainFixes #1142 | Workflow:
d6b71842Summary
Adds explicit latent-heat support for discrete (PMF) and continuous (PDF) binned particle distributions. The latent-heat step now mirrors isothermal shape handling, validates concentrations/volumes, and keeps latent-heat energy diagnostics consistent across all distribution types. Extensive binned fixtures and tests cover mass conservation, isothermal parity, energy bookkeeping, shapes, and error paths.
What Changed
New Components
Modified Components
particula/dynamics/condensation/condensation_strategies.py– Added shape-normalization helper reused by the latent-heat step, validated concentration/volume inputs, and aligned per-species mass transfer application and latent-heat energy tracking with the isothermal path for 1D/2D binned inputs.particula/dynamics/condensation/tests/condensation_strategies_test.py– Added PMF/PDF binned fixtures (10/50/100 bins) plus mass-conservation, isothermal-parity, energy-tracking, shape, and error-handling tests for latent-heat condensation.adw-docs/dev-plans/*– Updated epic/feature plan indexes for E5 latent-heat work.Tests Added/Updated
rate/mass_transfer_rateand step outputs on binned inputs.How It Works
The latent-heat step now enforces the same shape logic as the isothermal path and validates inputs before applying updates:
Implementation Notes
CondensationIsothermal.step(), trimming or broadcasting columns and reshaping 1D transfers for binned species counts._calculate_norm_concnow guards volume/concentration finiteness to keep-Werrorruns clean.PresetParticleRadiusBuilderwith pmf/pdf distributions, multi-species densities, and concentration-weighted totals to verify conservation and parity.Testing