fix: #884 - [E2-F2-P1] Create EquilibriaStrategy ABC and LiquidVaporPartitioningStrategy#892
Conversation
Successfully fixed: - Added EquilibriaStrategy and PhaseConcentrations/EquilibriumResult dataclasses to capture structured outputs - Wrapped liquid-vapor partitioning in LiquidVaporPartitioningStrategy with input validation, error propagation, and conversion helpers - Exported the strategy API and extended partitioning tests to cover new validation paths and dataclasses Still failing: - None ADW-ID: 4a730a35
Include the additional helper outputs in partitioning test fixtures so formatting and docstrings stay aligned with lint rules. Closes uncscode#884 ADW-ID: 4a730a35
There was a problem hiding this comment.
Pull request overview
This PR introduces a Strategy pattern architecture to the equilibria module by creating an EquilibriaStrategy abstract base class and implementing LiquidVaporPartitioningStrategy as the first concrete strategy. The new design wraps existing partitioning functions with structured dataclass outputs (EquilibriumResult and PhaseConcentrations), providing a clean API for future builders and factories while maintaining compatibility with existing functionality.
Changes:
- Created
EquilibriaStrategyABC with abstractsolve()method and concreteget_name()method - Implemented
LiquidVaporPartitioningStrategythat wraps liquid-vapor partitioning with water activity configuration - Added
PhaseConcentrationsandEquilibriumResultdataclasses for structured outputs - Extended
_build_partitioning_inputshelper to return oxygen2carbon and density for strategy tests - Exported new classes in
equilibria/__init__.py
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
particula/equilibria/equilibria_strategies.py |
Defines the ABC, dataclasses, and concrete liquid-vapor strategy with validation and conversion helpers |
particula/equilibria/tests/equilibria_strategies_test.py |
Comprehensive tests covering initialization, water activity validation, shape checking, regression comparison, error propagation, and edge cases |
particula/equilibria/tests/partitioning_test.py |
Updated existing tests to handle additional return values (oxygen2carbon, density) from modified helper function |
particula/equilibria/__init__.py |
Added exports for new strategy classes and dataclasses |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| beta_phase = self._to_phase(beta) if beta is not None else None | ||
|
|
||
| if ( | ||
| partition_coefficients.shape |
There was a problem hiding this comment.
The condition on line 211 is redundant. The expression partition_coefficients.shape and partition_coefficients.shape != alpha_phase.species_concentrations.shape will always evaluate the second part when the first part is truthy, because a non-empty tuple is always truthy in Python. The first partition_coefficients.shape check doesn't add meaningful protection. Consider simplifying to just if partition_coefficients.shape != alpha_phase.species_concentrations.shape: or if you need to handle scalar cases, use if partition_coefficients.ndim > 0 and partition_coefficients.shape != alpha_phase.species_concentrations.shape:
| partition_coefficients.shape | |
| partition_coefficients.ndim > 0 |
| @staticmethod | ||
| def _extract_error( | ||
| system: SystemOutput, fit_result: OptimizeResult | ||
| ) -> float: | ||
| if len(system) >= 4 and np.isfinite(system[3]): | ||
| return float(system[3]) | ||
| fit_value = getattr(fit_result, "fun", np.nan) | ||
| if np.isfinite(fit_value): | ||
| return float(fit_value) | ||
| raise ValueError("Unable to determine finite error from system output") |
There was a problem hiding this comment.
The error handling case where both system[3] and fit_result.fun are non-finite is not covered by tests. Consider adding a test case that verifies the ValueError is raised when both error sources are NaN or infinite, to ensure this edge case is properly handled.
| def test_convert_to_result_rejects_malformed_tuple(inputs): | ||
| """Reject malformed phase/system tuples in the conversion helper.""" | ||
| ( | ||
| c_star_j_dry, |
There was a problem hiding this comment.
Variable c_star_j_dry is not used.
| c_star_j_dry, | |
| _c_star_j_dry, |
| """Reject malformed phase/system tuples in the conversion helper.""" | ||
| ( | ||
| c_star_j_dry, | ||
| concentration_organic_matter, |
There was a problem hiding this comment.
Variable concentration_organic_matter is not used.
| concentration_organic_matter, | |
| _, |
| ( | ||
| c_star_j_dry, | ||
| concentration_organic_matter, | ||
| molar_mass, |
There was a problem hiding this comment.
Variable molar_mass is not used.
| molar_mass, | |
| _molar_mass, |
| c_star_j_dry, | ||
| concentration_organic_matter, | ||
| molar_mass, | ||
| oxygen2carbon, |
There was a problem hiding this comment.
Variable oxygen2carbon is not used.
| oxygen2carbon, | |
| _oxygen2carbon, |
| oxygen2carbon, | ||
| density, |
There was a problem hiding this comment.
Variable density is not used.
| oxygen2carbon, | |
| density, | |
| _oxygen2carbon, | |
| _density, |
| molar_mass, | ||
| oxygen2carbon, | ||
| density, | ||
| _gamma, |
There was a problem hiding this comment.
Variable _gamma is not used.
| oxygen2carbon, | ||
| density, | ||
| _gamma, | ||
| _mass_fraction, |
There was a problem hiding this comment.
Variable _mass_fraction is not used.
| _mass_fraction, | |
| _, |
| density, | ||
| _gamma, | ||
| _mass_fraction, | ||
| _q_ab, |
There was a problem hiding this comment.
Variable _q_ab is not used.
Co-authored-by: Gorkowski <10118307+Gorkowski@users.noreply.github.com>
Target Branch:
mainFixes #884 | Workflow:
4a730a35Summary
Introduces the EquilibriaStrategy ABC and LiquidVaporPartitioningStrategy to give the equilibria module a Strategy-pattern entry point with structured outputs. The new strategy wraps the refactored partitioning helpers, exposes typed dataclasses, and ships co-located tests so that future builders/factories can rely on a stable API.
What Changed
New Components
particula/equilibria/equilibria_strategies.py- DefinesPhaseConcentrations,EquilibriumResult, theEquilibriaStrategyABC, and theLiquidVaporPartitioningStrategyimplementation with validation, conversion helpers, and Google-style docstrings referencing Gorkowski et al. (2019).particula/equilibria/tests/equilibria_strategies_test.py- Exercises the new strategy with default/custom water activity, error propagation, beta-phase absence, malformed tuples, and regression comparisons to the existing partitioning helpers.Modified Components
particula/equilibria/tests/partitioning_test.py- Leaves the reusable_build_partitioning_inputshelper to continue supporting the new strategy tests (imports reorganized around the helper).Tests Added/Updated
particula/equilibria/tests/equilibria_strategies_test.py- Validates dataclasses,get_name(), water-activity bounds, conversion helpers, empty-input guard, and error propagation.How It Works
LiquidVaporPartitioningStrategy.solve()now orchestrates the helper routines and shapes their outputs into typed dataclasses:The conversion helper enforces tuple shapes, ensures species/value alignment, derives
PhaseConcentrations, and normalizes the error from either the system output or optimizer result.Implementation Notes
ValueErrors or propagate the finite error returned from the helper routines.Testing
pytest particula/equilibria/tests/equilibria_strategies_test.py