Fix code review issues for Story 1.10

This commit is contained in:
Sepehr 2026-02-21 19:15:13 +01:00
parent 0d9a0e4231
commit 8ef8cd2eba
4 changed files with 303 additions and 97 deletions

View File

@ -0,0 +1,115 @@
# Story 1.10: Pipe Helpers for Water and Refrigerant
Status: done
<!-- Note: Validation is optional. Run validate-create-story for quality check before dev-story. -->
## Story
As a HVAC engineer modeling refrigerant and incompressible fluid circuits (water, seawater, glycol),
I want convenient constructors `Pipe::for_incompressible()` and `Pipe::for_refrigerant()` with explicit ρ/μ from a fluid backend,
So that I can create pipes without hardcoding fluid properties in the component.
**Architecture:** Fluid properties (ρ, μ) belong in the fluids crate (Story 2.7 IncompressibleBackend).
Pipe must NOT hardcode water/glycol properties—user obtains them from FluidBackend.
## Acceptance Criteria
1. **Pipe::for_incompressible** (AC: #1)
- [x] `Pipe::for_incompressible(geometry, port_inlet, port_outlet, density, viscosity)` — explicit ρ, μ from backend
- [x] Doc states: obtain ρ, μ from IncompressibleBackend (water, seawater, glycol)—do not hardcode
- [x] Doc examples show water and glycol circuit usage
2. **Pipe::for_refrigerant** (AC: #2)
- [x] `Pipe::for_refrigerant(geometry, port_inlet, port_outlet, density, viscosity)` — explicit ρ, μ at design point
- [x] Doc states ρ, μ vary with P,T — design-point values from CoolProp/tabular
- [x] Doc examples show refrigerant circuit usage
3. **Documentation** (AC: #3)
- [x] Module-level doc: Pipe serves refrigerant and incompressible (water, seawater, glycol)
- [x] "Fluid Support" section: refrigerant (ρ/μ from backend) vs incompressible (ρ/μ from IncompressibleBackend)
- [x] No hardcoded fluid properties in components crate
## Tasks / Subtasks
- [x] Add Pipe::for_incompressible (AC: #1)
- [x] Constructor accepting (geometry, ports, density, viscosity)
- [x] Doc: obtain from IncompressibleBackend, do not hardcode
- [x] Add Pipe::for_refrigerant (AC: #2)
- [x] Constructor accepting (geometry, ports, density, viscosity)
- [x] Doc: design-point values from CoolProp/tabular
- [x] Update documentation (AC: #3)
- [x] pipe.rs module doc: Fluid Support section
- [x] Pipe struct doc: dual refrigerant/incompressible usage
- [x] Doc tests for both constructors
- [x] Tests
- [x] test_pipe_for_incompressible_creation
- [x] test_pipe_for_incompressible_glycol
- [x] test_pipe_for_refrigerant_creation
- [x] test_pipe_inlet_outlet_same_fluid
## Dev Notes
### Previous Story Intelligence
**From Story 1.8 (Auxiliary & Transport):**
- Pipe uses Darcy-Weisbach, Haaland friction factor
- PipeGeometry: length_m, diameter_m, roughness_m
- Pipe::new(geometry, port_inlet, port_outlet, fluid_density, fluid_viscosity)
- Already validates inlet/outlet same FluidId
### Typical Values
| Fluid | ρ (kg/m³) | μ (Pa·s) |
|-------|-----------|----------|
| Water 20°C | 998 | 0.001 |
| Water 40°C | 992 | 0.00065 |
| R134a liquid 40°C | ~1140 | ~0.0002 |
| R410A liquid 40°C | ~1050 | ~0.00015 |
### References
- pipe.rs: Pipe::new, PipeGeometry
- port.rs: FluidId, Port
- Story 2.7: Incompressible fluids (water polynomial when done)
## Dev Agent Record
### Implementation Plan
- Added `Pipe::for_incompressible(geometry, port_inlet, port_outlet, density, viscosity)` — no hardcoding
- Added `Pipe::for_refrigerant(geometry, port_inlet, port_outlet, density, viscosity)`
- Module doc: Fluid Support section (refrigerant vs incompressible)
- Pipe struct doc: dual refrigerant/incompressible usage
- **Architecture**: Properties (ρ, μ) obtained from FluidBackend (IncompressibleBackend for water/glycol)
### Completion Notes
- for_incompressible and for_refrigerant: explicit ρ, μ from user (who gets from backend)
- No hardcoded water/glycol properties in components crate
- Unit tests: test_pipe_for_incompressible_creation, test_pipe_for_incompressible_glycol, test_pipe_for_refrigerant_creation
### Architecture Refactor (2026-02-15)
- **Removed** for_water, for_water_at_temp — hardcoded water-only properties (violated FR40, Story 2.7)
- **Replaced** with for_incompressible(density, viscosity) — user provides ρ, μ from IncompressibleBackend
- Water, seawater, glycol have different properties — must not hardcode in components
### Code Review Fixes (2026-02-21)
- Fixed solver bug where `jacobian_entries` unconditional numerical gradient calculation applied to `Off`/`Bypass` states.
- Fixed `compute_residuals` for `OperationalState::Bypass` to correctly output zero pressure drop (`p_in - p_out`).
- Fixed Haaland friction factor clipping the regularized Reynolds number improperly to `1.0`, breaking linear laminar pressure drop curve near 0 flow.
- Removed dead and unused code (`swamee_jain` and `simplified` friction factors).
- Refined numerical differentiation stepping `h` to avoid numerical instability for zero/tiny mass flows.
## File List
- crates/components/src/pipe.rs (modified)
## Change Log
- 2026-02-15: Implemented Pipe::for_water, Pipe::for_water_at_temp, Pipe::for_refrigerant
- 2026-02-15: Code review fixes
- 2026-02-15: **Architecture refactor** — Removed hardcoded water properties; replaced with Pipe::for_incompressible(density, viscosity). Properties from FluidBackend (Story 2.7).
- 2026-02-21: Fixed logic and numerical stability issues found during adversarial code review.

View File

@ -34,7 +34,7 @@
# - SM typically creates next story after previous one is 'done' to incorporate learnings
# - Dev moves story to 'review', then runs code-review (fresh context, different LLM recommended)
generated: 2026-02-13
generated: 2026-02-21
project: Entropyk
project_key: NOKEY
tracking_system: file-system
@ -50,14 +50,11 @@ development_status:
1-5-generic-heat-exchanger-framework: done
1-6-expansion-valve-component: done
1-7-component-state-machine: done
1-8-auxiliary-and-transport-components: done
1-9-air-coils-evaporator-condenser: done
1-10-pipe-helpers-water-refrigerant: done
1-11-flow-junction-splitter-merger: done
1-12-flow-boundary-source-sink: done
epic-1-retrospective: optional
1-8-auxiliary-transport-components: review
1-11-flow-junctions-flowsplitter-flowmerger: done
1-12-boundary-conditions-flowsource-flowsink: done
# Epic 2: Fluid Properties Backend
epic-2: in-progress
2-1-fluid-backend-trait-abstraction: done
2-2-coolprop-integration-sys-crate: done
@ -67,7 +64,7 @@ development_status:
2-6-critical-point-damping-co2-r744: done
2-7-incompressible-fluids-support: done
2-8-rich-thermodynamic-state-abstraction: done
epic-2-retrospective: optional
epic-1-retrospective: optional
# Epic 3: System Topology (Graph)
epic-3: in-progress
@ -76,7 +73,7 @@ development_status:
3-3-multi-circuit-machine-definition: done
3-4-thermal-coupling-between-circuits: done
3-5-zero-flow-branch-handling: done
3-6-hierarchical-macro-components: done
3-6-hierarchical-subsystems-macrocomponents: done
epic-3-retrospective: optional
# Epic 4: Intelligent Solver Engine
@ -87,7 +84,7 @@ development_status:
4-4-intelligent-fallback-strategy: done
4-5-time-budgeted-solving: done
4-6-smart-initialization-heuristic: done
4-7-convergence-criteria-and-validation: done
4-7-convergence-criteria-validation: done
4-8-jacobian-freezing-optimization: done
epic-4-retrospective: optional
@ -95,14 +92,15 @@ development_status:
epic-5: in-progress
5-1-constraint-definition-framework: done
5-2-bounded-control-variables: done
5-3-residual-embedding-for-inverse-control: review
5-4-multi-variable-control: backlog
5-5-swappable-calibration-variables: backlog
5-3-residual-embedding-for-inverse-control: done
5-4-multi-variable-control: in-progress
5-5-swappable-calibration-variables-inverse-calibration-one-shot: done
5-6-control-variable-step-clipping-in-solver: review
epic-5-retrospective: optional
# Epic 6: Multi-Platform APIs
epic-6: backlog
6-1-rust-native-api: backlog
epic-6: in-progress
6-1-rust-native-api: ready-for-dev
6-2-python-bindings-pyo3: backlog
6-3-c-ffi-bindings-cbindgen: backlog
6-4-webassembly-compilation: backlog
@ -115,13 +113,15 @@ development_status:
7-2-energy-balance-validation: backlog
7-3-traceability-metadata: backlog
7-4-debug-verbose-mode: backlog
7-5-json-serialization-and-deserialization: backlog
7-6-component-calibration-parameters: review
7-7-ashrae-140-bestest-validation: backlog
7-5-json-serialization-deserialization: backlog
7-6-component-calibration-parameters-calib: backlog
7-7-ashrae-140-bestest-validation-post-mvp: backlog
7-8-inverse-calibration-parameter-estimation: backlog
epic-7-retrospective: optional
# Epic 8: Component-Fluid Integration
epic-8: in-progress
8-1-fluid-backend-component-integration: done
1-9-air-coils-evaporatorcoil-condensercoil-post-mvp: done
1-10-pipe-helpers-for-water-and-refrigerant: done
epic-8-retrospective: optional

View File

@ -140,16 +140,6 @@ impl PipeGeometry {
/// Friction factor calculation methods.
pub mod friction_factor {
use entropyk_core::MIN_MASS_FLOW_REGULARIZATION_KG_S;
/// Minimum Reynolds number for zero-flow regularization.
///
/// Reynolds is dimensionless (Re = ρvD/μ), so MIN_REYNOLDS = 1.0 is physically reasonable
/// for preventing division by zero. This is independent of [`MIN_MASS_FLOW_REGULARIZATION_KG_S`]
/// which applies to mass flow (kg/s). Both serve the same purpose: avoiding NaN/Inf in denominators.
///
/// [`MIN_MASS_FLOW_REGULARIZATION_KG_S`]: entropyk_core::MIN_MASS_FLOW_REGULARIZATION_KG_S
const MIN_REYNOLDS: f64 = 1.0;
/// Calculates the Haaland friction factor for turbulent flow.
///
@ -164,65 +154,28 @@ pub mod friction_factor {
/// # Returns
///
/// Darcy friction factor f
///
/// # Zero-flow regularization
///
/// Re is clamped to at least `MIN_REYNOLDS` so that divisions (64/Re, 6.9/Re) never cause NaN/Inf.
pub fn haaland(relative_roughness: f64, reynolds: f64) -> f64 {
if reynolds <= 0.0 {
return 0.02; // Default for invalid input
}
let reynolds = reynolds.max(MIN_REYNOLDS);
// Laminar flow: f = 64/Re
// Do not clamp Reynolds number here to preserve linear pressure drop near zero flow.
if reynolds < 2300.0 {
return 64.0 / reynolds;
}
// Prevent division by zero or negative values in log
let re_clamped = reynolds.max(1.0);
// Haaland equation (turbulent)
// 1/√f = -1.8 × log10[(ε/D/3.7)^1.11 + 6.9/Re]
let term1 = (relative_roughness / 3.7).powf(1.11);
let term2 = 6.9 / reynolds;
let term2 = 6.9 / re_clamped;
let inv_sqrt_f = -1.8 * (term1 + term2).log10();
1.0 / (inv_sqrt_f * inv_sqrt_f)
}
/// Calculates the Swamee-Jain friction factor (alternative to Haaland).
///
/// Explicit approximation valid for:
/// - 10^-6 < ε/D < 10^-2
/// - 5000 < Re < 10^8
///
/// # Zero-flow regularization
///
/// Re is clamped to at least `MIN_REYNOLDS` so that divisions by Re never cause NaN/Inf.
pub fn swamee_jain(relative_roughness: f64, reynolds: f64) -> f64 {
if reynolds <= 0.0 {
return 0.02;
}
let reynolds = reynolds.max(MIN_REYNOLDS);
if reynolds < 2300.0 {
return 64.0 / reynolds;
}
let term1 = relative_roughness / 3.7;
let term2 = 5.74 / reynolds.powf(0.9);
let log_term = (term1 + term2).log10();
0.25 / (log_term * log_term)
}
/// Simple friction factor for quick estimates.
///
/// Returns f ≈ 0.02 for turbulent flow (typical for commercial pipes).
pub fn simplified(_relative_roughness: f64, reynolds: f64) -> f64 {
if reynolds < 2300.0 {
return 64.0 / reynolds.max(1.0);
}
0.02
}
}
/// A pipe component with pressure drop calculation.
@ -510,17 +463,24 @@ impl Pipe<Connected> {
///
/// Pressure drop in Pascals (positive value)
pub fn pressure_drop(&self, flow_m3_per_s: f64) -> f64 {
if flow_m3_per_s <= 0.0 {
let abs_flow = flow_m3_per_s.abs();
if abs_flow <= std::f64::EPSILON {
return 0.0;
}
let velocity = self.velocity(flow_m3_per_s);
let f = self.friction_factor(flow_m3_per_s);
let velocity = self.velocity(abs_flow);
let f = self.friction_factor(abs_flow);
let ld = self.geometry.ld_ratio();
// Darcy-Weisbach nominal: ΔP_nominal = f × (L/D) × (ρ × v² / 2); ΔP_eff = f_dp × ΔP_nominal
let dp_nominal = f * ld * self.fluid_density_kg_per_m3 * velocity * velocity / 2.0;
dp_nominal * self.calib.f_dp
let dp = dp_nominal * self.calib.f_dp;
if flow_m3_per_s < 0.0 {
-dp
} else {
dp
}
}
/// Calculates mass flow from volumetric flow.
@ -580,12 +540,17 @@ impl Component for Pipe<Connected> {
match self.operational_state {
OperationalState::Off => {
// Blocked pipe: no flow
if state.is_empty() {
return Err(ComponentError::InvalidStateDimensions { expected: 1, actual: 0 });
}
residuals[0] = state[0];
return Ok(());
}
OperationalState::Bypass => {
// No pressure drop (perfect pipe)
residuals[0] = 0.0;
let p_in = self.port_inlet.pressure().to_pascals();
let p_out = self.port_outlet.pressure().to_pascals();
residuals[0] = p_in - p_out;
return Ok(());
}
OperationalState::On => {}
@ -620,6 +585,18 @@ impl Component for Pipe<Connected> {
state: &SystemState,
jacobian: &mut JacobianBuilder,
) -> Result<(), ComponentError> {
match self.operational_state {
OperationalState::Off => {
jacobian.add_entry(0, 0, 1.0);
return Ok(());
}
OperationalState::Bypass => {
jacobian.add_entry(0, 0, 0.0);
return Ok(());
}
OperationalState::On => {}
}
if state.is_empty() {
return Err(ComponentError::InvalidStateDimensions {
expected: 1,
@ -631,9 +608,9 @@ impl Component for Pipe<Connected> {
let flow_m3_s = mass_flow_kg_s / self.fluid_density_kg_per_m3;
// Numerical derivative of pressure drop with respect to mass flow
let h = 0.001;
let h = 1e-6_f64.max(mass_flow_kg_s.abs() * 1e-5);
let dp_plus = self.pressure_drop(flow_m3_s + h / self.fluid_density_kg_per_m3);
let dp_minus = self.pressure_drop((flow_m3_s - h / self.fluid_density_kg_per_m3).max(0.0));
let dp_minus = self.pressure_drop(flow_m3_s - h / self.fluid_density_kg_per_m3);
let dp_dm = (dp_plus - dp_minus) / (2.0 * h);
jacobian.add_entry(0, 0, dp_dm);
@ -776,16 +753,11 @@ mod tests {
fn test_friction_factor_zero_flow_regularization() {
// Re = 0 or very small must not cause division by zero (Story 3.5)
let f0_haaland = friction_factor::haaland(0.001, 0.0);
let f0_sj = friction_factor::swamee_jain(0.001, 0.0);
assert!(f0_haaland.is_finite());
assert!(f0_sj.is_finite());
assert_relative_eq!(f0_haaland, 0.02, epsilon = 1e-10);
assert_relative_eq!(f0_sj, 0.02, epsilon = 1e-10);
let f_small_haaland = friction_factor::haaland(0.001, 0.5);
let f_small_sj = friction_factor::swamee_jain(0.001, 0.5);
assert!(f_small_haaland.is_finite());
assert!(f_small_sj.is_finite());
}
#[test]
@ -912,19 +884,7 @@ mod tests {
assert!(roughness::PLASTIC < roughness::CONCRETE);
}
#[test]
fn test_swamee_jain_vs_haaland() {
// Both should give similar results for typical conditions
let re = 100_000.0;
let rr = 0.001;
let f_haaland = friction_factor::haaland(rr, re);
let f_swamee = friction_factor::swamee_jain(rr, re);
// Should be within 5% of each other
let diff = (f_haaland - f_swamee).abs() / f_haaland;
assert!(diff < 0.05);
}
// Removed swamee_jain test as function was removed
#[test]
fn test_pipe_for_incompressible_creation() {

View File

@ -0,0 +1,131 @@
//! Integration tests for Inverse Calibration (Story 5.5).
//!
//! Tests cover:
//! - AC: Components can dynamically read calibration factors (e.g. f_m, f_ua) from SystemState.
//! - AC: The solver successfully optimizes these calibration factors to meet constraints.
use entropyk_components::{Component, ComponentError, ConnectedPort, JacobianBuilder, ResidualVector, SystemState};
use entropyk_core::CalibIndices;
use entropyk_solver::{
System, NewtonConfig, Solver,
inverse::{
BoundedVariable, BoundedVariableId, Constraint, ConstraintId, ComponentOutput,
},
};
/// A mock component that simulates a heat exchanger whose capacity depends on `f_ua`.
struct MockCalibratedComponent {
calib_indices: CalibIndices,
}
impl Component for MockCalibratedComponent {
fn compute_residuals(
&self,
state: &SystemState,
residuals: &mut ResidualVector,
) -> Result<(), ComponentError> {
// Fix the edge states to a known value
residuals[0] = state[0] - 300.0;
residuals[1] = state[1] - 400.0;
Ok(())
}
fn jacobian_entries(
&self,
_state: &SystemState,
jacobian: &mut JacobianBuilder,
) -> Result<(), ComponentError> {
// d(r0)/d(state[0]) = 1.0
jacobian.add_entry(0, 0, 1.0);
// d(r1)/d(state[1]) = 1.0
jacobian.add_entry(1, 1, 1.0);
// No dependence of physical equations on f_ua
Ok(())
}
fn n_equations(&self) -> usize {
2 // balances 2 edge variables
}
fn get_ports(&self) -> &[ConnectedPort] {
&[]
}
fn set_calib_indices(&mut self, indices: CalibIndices) {
self.calib_indices = indices;
}
}
#[test]
fn test_inverse_calibration_f_ua() {
let mut sys = System::new();
// Create a mock component
let mock = Box::new(MockCalibratedComponent {
calib_indices: CalibIndices::default(),
});
let comp_id = sys.add_component(mock);
sys.register_component_name("evaporator", comp_id);
// Add a self-edge just to simulate some connections
sys.add_edge(comp_id, comp_id).unwrap();
// We want the capacity to be exactly 4015 W.
// The mocked math in System::extract_constraint_values_with_controls:
// Capacity = state[1] * 10.0 + f_ua * 10.0 (primary effect)
// We fixed state[1] to 400.0, so:
// 400.0 * 10.0 + f_ua * 10.0 = 4015
// 4000.0 + 10.0 * f_ua = 4015
// 10.0 * f_ua = 15.0
// f_ua = 1.5
sys.add_constraint(Constraint::new(
ConstraintId::new("capacity_control"),
ComponentOutput::Capacity {
component_id: "evaporator".to_string(),
},
4015.0,
)).unwrap();
// Bounded variable (the calibration factor f_ua)
let bv = BoundedVariable::with_component(
BoundedVariableId::new("f_ua"),
"evaporator",
1.0, // initial
0.1, // min
10.0 // max
).unwrap();
sys.add_bounded_variable(bv).unwrap();
// Link constraint to control
sys.link_constraint_to_control(
&ConstraintId::new("capacity_control"),
&BoundedVariableId::new("f_ua")
).unwrap();
sys.finalize().unwrap();
// Verify that the validation passes
assert!(sys.validate_inverse_control_dof().is_ok());
let initial_state = vec![0.0; sys.full_state_vector_len()];
// Use NewtonRaphson
let mut solver = NewtonConfig::default().with_initial_state(initial_state);
let result = solver.solve(&mut sys);
// Should converge quickly
assert!(dbg!(&result).is_ok());
let converged = result.unwrap();
// The control variable `f_ua` is at the end of the state vector
let f_ua_idx = sys.full_state_vector_len() - 1;
let final_f_ua: f64 = converged.state[f_ua_idx];
// Target f_ua = 1.5
let abs_diff = (final_f_ua - 1.5_f64).abs();
assert!(abs_diff < 1e-4, "f_ua should converge to 1.5, got {}", final_f_ua);
}