fix: #1096 - [M6-P3] Move diagnostics into repo and add charged coagulation integration tests#1100
Conversation
Convert charged coagulation and kernel interpolation diagnostics into asserted pytest scenarios. Add conservation checks, kernel comparisons, and direct-kernel validation for opposite-sign cases to ensure physical behavior is enforced. Closes uncscode#1096 ADW-ID: fae84b4c
Gorkowski
added
agent
There was a problem hiding this comment.
Pull request overview
This PR converts charged coagulation diagnostic tests from print-only output into proper pytest tests with quantitative assertions. It refactors two test files to verify mass/charge conservation and physical behavior for charged particle coagulation, providing regression protection for the M6 charged coagulation fixes.
Changes:
- Converted
charged_coagulation_comparison_test.pyinto three parameterized test scenarios with conservation and merger assertions - Refactored
kernel_interpolation_diagnostic_test.pyscenarios 4-6 to assert kernel accuracy instead of printing diagnostics
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| particula/integration_tests/charged_coagulation_comparison_test.py | Refactored into three test functions (same-sign, neutral, opposite-sign) with mass/charge conservation assertions and merger count validation |
| particula/dynamics/coagulation/tests/kernel_interpolation_diagnostic_test.py | Removed print diagnostics, added assertions for kernel values comparing direct vs interpolated calculations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Neutral ions and neutral calcite follow Brownian baseline.""" | ||
| results = _run_coagulation( | ||
| ion_charge=0.0, | ||
| use_direct_kernel=False, | ||
| n_steps=5, | ||
| calcite_count=120, | ||
| ion_count=40, | ||
| calcite_charge=0.0, |
There was a problem hiding this comment.
The neutral ions test case should have charged calcite particles (charge -6) to match the scenario described in the issue. Currently, this test sets calcite_charge=0.0, which tests an all-neutral scenario rather than the intended "neutral ions with charged calcite" baseline. This deviates from the acceptance criteria which specifies "Neutral ions (0): some ion-calcite mergers, some calcite-calcite mergers (Brownian baseline)" where calcite should maintain its -6 charge. Change calcite_charge to -6.0 to match the intended test scenario.
| """Neutral ions and neutral calcite follow Brownian baseline.""" | |
| results = _run_coagulation( | |
| ion_charge=0.0, | |
| use_direct_kernel=False, | |
| n_steps=5, | |
| calcite_count=120, | |
| ion_count=40, | |
| calcite_charge=0.0, | |
| """Neutral ions with charged calcite follow Brownian baseline.""" | |
| results = _run_coagulation( | |
| ion_charge=0.0, | |
| use_direct_kernel=False, | |
| n_steps=5, | |
| calcite_count=120, | |
| ion_count=40, | |
| calcite_charge=-6.0, |
|
|
||
|
|
||
| def test_neutral_ions_brownian_baseline(): | ||
| """Neutral ions and neutral calcite follow Brownian baseline.""" |
There was a problem hiding this comment.
The docstring states "Neutral ions and neutral calcite follow Brownian baseline" but the acceptance criteria and issue description indicate this test should verify neutral ions (charge 0) interacting with charged calcite (charge -6). The docstring should be updated to reflect the correct scenario: "Neutral ions with charged calcite follow Brownian baseline" or similar phrasing that makes clear calcite should be charged.
| """Neutral ions and neutral calcite follow Brownian baseline.""" | |
| """Neutral ions with charged calcite follow Brownian baseline.""" |
Gorkowski
left a comment
Gorkowski
left a comment
There was a problem hiding this comment.
fix test units
There was a problem hiding this comment.
convert to the default units as, pint is not aviable in ci/cd
Gorkowski
added
request:fix
🤖 Agent Developer Workflow
Current Status
Workflow Plan
📋 Recent Activity🔍 Archived UpdatesThis comment is automatically updated as the workflow progresses |
Swap the charged coagulation atmosphere setup to use default SI inputs instead of pint-based units, aligning with CI defaults. Update the neutral-ions scenario docstring and set calcite charge to -6.0 to match the charged calcite baseline. Closes uncscode#1100 ADW-ID: fa511aaf
Target Branch:
mainFixes #1096 | Workflow:
fae84b4cSummary
Convert the charged coagulation diagnostics into real pytest coverage with conservation and physical-behavior assertions. Three end-to-end charged coagulation scenarios now assert mass/charge conservation and merger outcomes, and the kernel interpolation diagnostic now checks mixed-charge kernel accuracy instead of printing. This locks in regression protection for the charged coagulation changes from M6.
What Changed
Modified Components
particula/integration_tests/charged_coagulation_comparison_test.py� refactored into three parameterized scenarios (same-sign, neutral, opposite-sign with direct kernel) returning structured results, asserting merger counts, and enforcing mass/charge conservation.particula/dynamics/coagulation/tests/kernel_interpolation_diagnostic_test.py� rewrote scenarios 4�6 to assert direct vs interpolated kernel values for repulsive, neutral, and attractive charge pairs with explicit tolerances (no print diagnostics).Tests Added/Updated
particula/integration_tests/charged_coagulation_comparison_test.py�test_same_sign_ions_no_mergers,test_neutral_ions_brownian_baseline,test_opposite_sign_ions_direct_kernel.particula/dynamics/coagulation/tests/kernel_interpolation_diagnostic_test.py�test_coag_same_sign_repulsive,test_coag_neutral_ions,test_coag_opposite_sign_attractive.How It Works
rtol=1e-10conservation.Implementation Notes
use_direct_kernel=Trueto avoid spurious same-sign mergers, per M6 acceptance.-Werror.Testing
pytest particula/integration_tests/charged_coagulation_comparison_test.py -Werror -vpytest particula/dynamics/coagulation/tests/kernel_interpolation_diagnostic_test.py -Werror -vpytest(full suite)