From 0ddc2e313953a38789f04c596e51bd6ee104ea7b Mon Sep 17 00:00:00 2001 From: James Tomlinson Date: Wed, 26 Jun 2024 14:49:13 +0100 Subject: [PATCH 1/5] feat: Initial implementation of CBC solver. --- pywr-core/Cargo.toml | 1 + pywr-core/src/solvers/cbc/mod.rs | 338 ++++++++++++++++++ pywr-core/src/solvers/cbc/settings.rs | 92 +++++ pywr-core/src/solvers/mod.rs | 5 + pywr-core/src/test_utils.rs | 11 + .../src/timeseries/align_and_resample.rs | 6 +- 6 files changed, 448 insertions(+), 5 deletions(-) create mode 100644 pywr-core/src/solvers/cbc/mod.rs create mode 100644 pywr-core/src/solvers/cbc/settings.rs diff --git a/pywr-core/Cargo.toml b/pywr-core/Cargo.toml index 8ff77f88..7b2d23ac 100644 --- a/pywr-core/Cargo.toml +++ b/pywr-core/Cargo.toml @@ -51,6 +51,7 @@ serde = { version = "1.0.197", features = ["derive"] } criterion = "0.5" [features] +cbc = [] highs = ["dep:highs-sys"] ipm-ocl = ["dep:ipm-ocl", "dep:ocl"] ipm-simd = ["dep:ipm-simd"] diff --git a/pywr-core/src/solvers/cbc/mod.rs b/pywr-core/src/solvers/cbc/mod.rs new file mode 100644 index 00000000..f82c01da --- /dev/null +++ b/pywr-core/src/solvers/cbc/mod.rs @@ -0,0 +1,338 @@ +mod settings; + +use super::builder::SolverBuilder; +use crate::network::Network; +use crate::solvers::builder::BuiltSolver; +use crate::solvers::{Solver, SolverFeatures, SolverTimings}; +use crate::state::State; +use crate::timestep::Timestep; +use crate::PywrError; +use coin_or_sys::cbc::*; +use libc::{c_double, c_int}; +pub use settings::{CbcSolverSettings, CbcSolverSettingsBuilder}; +use std::ffi::{c_char, CString}; +use std::time::Instant; +use std::{ptr, slice}; +use thiserror::Error; + +#[derive(Error, Debug, PartialEq, Eq)] +pub enum CbcError { + #[error("an unknown error occurred in Cbc.")] + UnknownError, +} + +pub type CoinBigIndex = c_int; + +struct Cbc { + ptr: *mut Cbc_Model, +} + +unsafe impl Send for Cbc {} + +impl Default for Cbc { + fn default() -> Self { + let model: Cbc; + + unsafe { + let ptr = Cbc_newModel(); + model = Cbc { ptr }; + Cbc_setLogLevel(ptr, 0); + Cbc_setObjSense(ptr, 1.0); + } + model + } +} + +impl Cbc { + #[allow(dead_code)] + pub fn print(&mut self) { + unsafe { + let prefix = CString::new(" ").expect("CString::new failed"); + Cbc_printModel(self.ptr, prefix.as_ptr()); + } + } + + pub fn change_row_lower(&mut self, row_lower: &[c_double]) { + for (i, val) in row_lower.iter().enumerate() { + unsafe { + Cbc_setRowLower(self.ptr, i as c_int, *val); + } + } + } + + pub fn change_row_upper(&mut self, row_upper: &[c_double]) { + for (i, val) in row_upper.iter().enumerate() { + unsafe { + Cbc_setRowUpper(self.ptr, i as c_int, *val); + } + } + } + + pub fn change_objective_coefficients(&mut self, obj_coefficients: &[c_double]) { + for (i, val) in obj_coefficients.iter().enumerate() { + unsafe { + Cbc_setObjCoeff(self.ptr, i as c_int, *val); + } + } + } + + pub fn add_cols(&mut self, col_lower: &[c_double], col_upper: &[c_double], obj_coefs: &[c_double]) { + let number: c_int = col_lower.len() as c_int; + + for col_idx in 0..number { + let lower = col_lower[col_idx as usize]; + let upper = col_upper[col_idx as usize]; + let obj_coef = obj_coefs[col_idx as usize]; + + unsafe { + let c_name = CString::new("col").expect("Failed to create CString for column name."); + Cbc_addCol( + self.ptr, + c_name.as_ptr(), + lower, + upper, + obj_coef, + 0 as c_char, + 0, + ptr::null_mut(), + ptr::null_mut(), + ); + } + } + } + + pub fn add_rows( + &mut self, + row_lower: &[c_double], + row_upper: &[c_double], + row_starts: &[CoinBigIndex], + columns: &[c_int], + elements: &[c_double], + ) { + let number: c_int = row_lower.len() as c_int; + + for row_idx in 0..number { + let start = row_starts[row_idx as usize]; + let end = row_starts[row_idx as usize + 1]; + let nz = end - start; + let cols = &columns[start as usize..end as usize]; + let coefs = &elements[start as usize..end as usize]; + + unsafe { + let c_name = CString::new("row").expect("Failed to create CString for column name."); + let sense = 'E'; + Cbc_addRow( + self.ptr, + c_name.as_ptr(), + nz, + cols.as_ptr(), + coefs.as_ptr(), + sense as c_char, + 0.0, + ); + + Cbc_setRowUpper(self.ptr, row_idx, row_upper[row_idx as usize]); + Cbc_setRowLower(self.ptr, row_idx, row_lower[row_idx as usize]); + } + } + } + + fn solve(&mut self) { + unsafe { + Cbc_solve(self.ptr); + } + } + + fn primal_column_solution(&mut self, number: usize) -> Vec { + let solution: Vec; + unsafe { + let data_ptr = Cbc_getColSolution(self.ptr); + solution = slice::from_raw_parts(data_ptr, number).to_vec() + } + solution + } + + #[allow(dead_code)] + fn get_objective_coefficients(&mut self, number: usize) -> Vec { + let coef: Vec; + unsafe { + let data_ptr = Cbc_getObjCoefficients(self.ptr); + coef = slice::from_raw_parts(data_ptr, number).to_vec() + } + coef + } + + #[allow(dead_code)] + fn get_row_upper(&mut self, number: usize) -> Vec { + let ub: Vec; + unsafe { + let data_ptr = Cbc_getRowUpper(self.ptr); + ub = slice::from_raw_parts(data_ptr, number).to_vec() + } + ub + } + + #[allow(dead_code)] + fn objective_value(&self) -> c_double { + unsafe { Cbc_getObjValue(self.ptr) } + } +} + +pub struct CbcSolver { + builder: BuiltSolver, + cbc: Cbc, +} + +impl CbcSolver { + fn from_builder(builder: BuiltSolver) -> Self { + let mut cbc = Cbc::default(); + + cbc.add_cols(builder.col_lower(), builder.col_upper(), builder.col_obj_coef()); + + cbc.add_rows( + builder.row_lower(), + builder.row_upper(), + builder.row_starts(), + builder.columns(), + builder.elements(), + ); + + CbcSolver { builder, cbc } + } + + fn solve(&mut self) -> Vec { + self.cbc.solve(); + + let num_cols = self.builder.num_cols() as usize; + + self.cbc.primal_column_solution(num_cols) + } +} + +impl Solver for CbcSolver { + type Settings = CbcSolverSettings; + + fn features() -> &'static [SolverFeatures] { + &[ + SolverFeatures::AggregatedNode, + SolverFeatures::AggregatedNodeFactors, + SolverFeatures::VirtualStorage, + ] + } + + fn setup(model: &Network, _settings: &Self::Settings) -> Result, PywrError> { + let builder = SolverBuilder::default(); + let built = builder.create(model)?; + + let solver = CbcSolver::from_builder(built); + Ok(Box::new(solver)) + } + + fn solve(&mut self, model: &Network, timestep: &Timestep, state: &mut State) -> Result { + let mut timings = SolverTimings::default(); + self.builder.update(model, timestep, state, &mut timings)?; + + let now = Instant::now(); + self.cbc.change_objective_coefficients(self.builder.col_obj_coef()); + timings.update_objective += now.elapsed(); + + let now = Instant::now(); + self.cbc.change_row_lower(self.builder.row_lower()); + self.cbc.change_row_upper(self.builder.row_upper()); + + for (row, column, coefficient) in self.builder.coefficients_to_update() { + // TODO raise an error for missing feature + // self.cbc.modify_coefficient(*row, *column, *coefficient) + } + + timings.update_constraints += now.elapsed(); + + let now = Instant::now(); + let solution = self.solve(); + timings.solve = now.elapsed(); + + // Create the updated network state from the results + let network_state = state.get_mut_network_state(); + network_state.reset(); + + let start_save_solution = Instant::now(); + for edge in model.edges().iter() { + let col = self.builder.col_for_edge(&edge.index()) as usize; + let flow = solution[col]; + // Round very small values to zero + let flow = if flow.abs() < 1e-10 { 0.0 } else { flow }; + network_state.add_flow(edge, timestep, flow)?; + } + network_state.complete(model, timestep)?; + timings.save_solution += start_save_solution.elapsed(); + + Ok(timings) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use float_cmp::approx_eq; + + #[test] + fn cbc_create() { + Cbc::default(); + } + + #[test] + fn cbc_add_rows() { + let mut model = Cbc::default(); + model.add_cols(&vec![0.0, 0.0], &vec![10.0, 10.0], &vec![0.0, 0.0]); + + let row_lower: Vec = vec![0.0]; + let row_upper: Vec = vec![2.0]; + let row_starts: Vec = vec![0, 2]; + let columns: Vec = vec![0, 1]; + let elements: Vec = vec![1.0, 1.0]; + + model.add_rows(&row_lower, &row_upper, &row_starts, &columns, &elements); + } + + #[test] + fn simple_solve() { + let row_upper = vec![10.0, 15.0]; + let row_lower = vec![0.0, 0.0]; + let col_lower = vec![0.0, 0.0, 0.0]; + let col_upper = vec![f64::MAX, f64::MAX, f64::MAX]; + let col_obj_coef = vec![-2.0, -3.0, -4.0]; + let row_starts = vec![0, 3, 6]; + let columns = vec![0, 1, 2, 0, 1, 2]; + let elements = vec![3.0, 2.0, 1.0, 2.0, 5.0, 3.0]; + + let mut lp = Cbc::default(); + + lp.add_cols(&col_lower, &col_upper, &col_obj_coef); + lp.add_rows(&row_lower, &row_upper, &row_starts, &columns, &elements); + lp.solve(); + + assert!(approx_eq!(f64, lp.objective_value(), -20.0)); + assert_eq!(lp.primal_column_solution(3), vec![0.0, 0.0, 5.0]); + } + + #[test] + fn solve_with_inf_row_bound() { + let row_upper = vec![10.0, f64::MAX]; + let row_lower = vec![0.0, 0.0]; + let col_lower = vec![0.0, 0.0, 0.0]; + let col_upper = vec![f64::MAX, f64::MAX, f64::MAX]; + let col_obj_coef = vec![-2.0, -3.0, -4.0]; + let row_starts = vec![0, 3, 6]; + let columns = vec![0, 1, 2, 0, 1, 2]; + let elements = vec![3.0, 2.0, 1.0, 2.0, 5.0, 3.0]; + + let mut lp = Cbc::default(); + + lp.add_cols(&col_lower, &col_upper, &col_obj_coef); + lp.add_rows(&row_lower, &row_upper, &row_starts, &columns, &elements); + lp.solve(); + + assert!(approx_eq!(f64, lp.objective_value(), -40.0)); + assert_eq!(lp.primal_column_solution(3), vec![0.0, 0.0, 10.0]); + } +} diff --git a/pywr-core/src/solvers/cbc/settings.rs b/pywr-core/src/solvers/cbc/settings.rs new file mode 100644 index 00000000..195663dd --- /dev/null +++ b/pywr-core/src/solvers/cbc/settings.rs @@ -0,0 +1,92 @@ +use crate::solvers::SolverSettings; + +/// Settings for the OpenCL IPM solvers. +/// +/// Create new settings using [`CbcSolverSettingsBuilder`] or use the default implementation; +#[derive(PartialEq, Debug, Copy, Clone)] +pub struct CbcSolverSettings { + parallel: bool, + threads: usize, +} + +// Default implementation is a convenience that defers to the builder. +impl Default for CbcSolverSettings { + fn default() -> Self { + CbcSolverSettingsBuilder::default().build() + } +} + +impl SolverSettings for CbcSolverSettings { + fn parallel(&self) -> bool { + self.parallel + } + + fn threads(&self) -> usize { + self.threads + } +} + +impl CbcSolverSettings { + /// Create a new builder for the settings + pub fn builder() -> CbcSolverSettingsBuilder { + CbcSolverSettingsBuilder::default() + } +} + +/// Builder for [`CbcSolverSettings`]. +/// +/// # Examples +/// +/// ``` +/// use std::num::NonZeroUsize; +/// use pywr_core::solvers::CbcSolverSettingsBuilder; +/// // Settings with parallel enabled and 4 threads. +/// let settings = CbcSolverSettingsBuilder::default().parallel().threads(4).build(); +/// +/// let mut builder = CbcSolverSettingsBuilder::default(); +/// +/// builder.parallel(); +/// let settings = builder.build(); +/// +/// ``` +#[derive(Default)] +pub struct CbcSolverSettingsBuilder { + parallel: bool, + threads: usize, +} + +impl CbcSolverSettingsBuilder { + pub fn parallel(&mut self) -> &mut Self { + self.parallel = true; + self + } + + pub fn threads(&mut self, threads: usize) -> &mut Self { + self.threads = threads; + self + } + + /// Construct a [`CbcSolverSettings`] from the builder. + pub fn build(&self) -> CbcSolverSettings { + CbcSolverSettings { + parallel: self.parallel, + threads: self.threads, + } + } +} + +#[cfg(test)] +mod tests { + use super::{CbcSolverSettings, CbcSolverSettingsBuilder}; + + #[test] + fn builder_test() { + let _settings = CbcSolverSettings { + parallel: true, + threads: 0, + }; + let settings_from_builder = CbcSolverSettingsBuilder::default().parallel().build(); + + assert_eq!(settings_from_builder, settings_from_builder); + } +} diff --git a/pywr-core/src/solvers/mod.rs b/pywr-core/src/solvers/mod.rs index b12e84a8..c2c192a8 100644 --- a/pywr-core/src/solvers/mod.rs +++ b/pywr-core/src/solvers/mod.rs @@ -6,6 +6,9 @@ use std::ops::{Add, AddAssign}; use std::time::Duration; mod builder; + +#[cfg(feature = "cbc")] +mod cbc; mod clp; mod col_edge_map; #[cfg(feature = "highs")] @@ -19,6 +22,8 @@ mod ipm_simd; pub use self::ipm_ocl::{ClIpmF32Solver, ClIpmF64Solver, ClIpmSolverSettings, ClIpmSolverSettingsBuilder}; #[cfg(feature = "ipm-simd")] pub use self::ipm_simd::{SimdIpmF64Solver, SimdIpmSolverSettings, SimdIpmSolverSettingsBuilder}; +#[cfg(feature = "cbc")] +pub use cbc::{CbcError, CbcSolver, CbcSolverSettings, CbcSolverSettingsBuilder}; pub use clp::{ClpError, ClpSolver, ClpSolverSettings, ClpSolverSettingsBuilder}; #[cfg(feature = "highs")] pub use highs::{HighsSolver, HighsSolverSettings, HighsSolverSettingsBuilder}; diff --git a/pywr-core/src/test_utils.rs b/pywr-core/src/test_utils.rs index ae3c85f4..dda2d43b 100644 --- a/pywr-core/src/test_utils.rs +++ b/pywr-core/src/test_utils.rs @@ -7,6 +7,8 @@ use crate::node::{Constraint, ConstraintValue, StorageInitialVolume}; use crate::parameters::{AggFunc, AggregatedParameter, Array2Parameter, ConstantParameter, Parameter}; use crate::recorders::AssertionRecorder; use crate::scenario::ScenarioGroupCollection; +#[cfg(feature = "cbc")] +use crate::solvers::CbcSolver; #[cfg(feature = "ipm-ocl")] use crate::solvers::ClIpmF64Solver; use crate::solvers::ClpSolver; @@ -197,6 +199,15 @@ pub fn run_all_solvers(model: &Model) { .run::(&Default::default()) .expect("Failed to solve with CLP"); + #[cfg(feature = "cbc")] + { + if model.check_solver_features::() { + model + .run::(&Default::default()) + .expect("Failed to solve with CBC"); + } + } + #[cfg(feature = "highs")] { if model.check_solver_features::() { diff --git a/pywr-schema/src/timeseries/align_and_resample.rs b/pywr-schema/src/timeseries/align_and_resample.rs index 6106829c..dde3e2f5 100644 --- a/pywr-schema/src/timeseries/align_and_resample.rs +++ b/pywr-schema/src/timeseries/align_and_resample.rs @@ -70,11 +70,7 @@ pub fn align_and_resample( // TODO: this does not extend the dataframe beyond its original end date. Should it do when using a forward fill strategy? // The df could be extend by the length of the duration it is being resampled to. df.clone() - .upsample::<[String; 0]>( - [], - "time", - Duration::parse(model_duration_string.as_str()), - )? + .upsample::<[String; 0]>([], "time", Duration::parse(model_duration_string.as_str()))? .fill_null(FillNullStrategy::Forward(None))? } Ordering::Equal => df, From a667f43da374be4961eeb22ce82f7d0f82e98a71 Mon Sep 17 00:00:00 2001 From: James Tomlinson Date: Tue, 2 Jul 2024 14:21:00 +0100 Subject: [PATCH 2/5] feat: Refactor run_all_solvers to have solvers without features. --- pyproject.toml | 10 ++++---- pywr-core/src/aggregated_node.rs | 2 +- pywr-core/src/network.rs | 6 ++--- pywr-core/src/recorders/mod.rs | 2 +- pywr-core/src/solvers/cbc/mod.rs | 18 ++++++------- pywr-core/src/solvers/clp/mod.rs | 4 +++ pywr-core/src/solvers/mod.rs | 1 + pywr-core/src/test_utils.rs | 43 +++++++++++++++++--------------- pywr-core/src/virtual_storage.rs | 6 ++--- 9 files changed, 50 insertions(+), 42 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 87b75d28..ba4a4d46 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -3,16 +3,16 @@ name = "pywr" version = "2.0.0beta" description = "" authors = [ - {name="James Tomlinson", email="tomo.bbe@gmail.com>"} + { name = "James Tomlinson", email = "tomo.bbe@gmail.com>" } ] readme = "README.md" requires-python = ">=3.9" license = "MIT OR Apache-2.0" dependencies = [ - "pandas", - "polars", - "pyarrow", - "click" + "pandas", + "polars", + "pyarrow", + "click" ] [build-system] diff --git a/pywr-core/src/aggregated_node.rs b/pywr-core/src/aggregated_node.rs index 1924dbc5..974f1697 100644 --- a/pywr-core/src/aggregated_node.rs +++ b/pywr-core/src/aggregated_node.rs @@ -347,6 +347,6 @@ mod tests { let model = Model::new(default_time_domain().into(), network); - run_all_solvers(&model); + run_all_solvers(&model, &["cbc"]); } } diff --git a/pywr-core/src/network.rs b/pywr-core/src/network.rs index 52aa3aa2..5309e577 100644 --- a/pywr-core/src/network.rs +++ b/pywr-core/src/network.rs @@ -1804,7 +1804,7 @@ mod tests { model.network_mut().add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } #[test] @@ -1828,7 +1828,7 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } /// Test proportional storage derived metric. @@ -1868,7 +1868,7 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } #[test] diff --git a/pywr-core/src/recorders/mod.rs b/pywr-core/src/recorders/mod.rs index 862869b7..c8dc508d 100644 --- a/pywr-core/src/recorders/mod.rs +++ b/pywr-core/src/recorders/mod.rs @@ -362,7 +362,7 @@ mod tests { let _idx = model.network_mut().add_recorder(Box::new(rec)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); // TODO fix this with respect to the trait. // let array = rec.data_view2().unwrap(); diff --git a/pywr-core/src/solvers/cbc/mod.rs b/pywr-core/src/solvers/cbc/mod.rs index f82c01da..7188d8fc 100644 --- a/pywr-core/src/solvers/cbc/mod.rs +++ b/pywr-core/src/solvers/cbc/mod.rs @@ -212,12 +212,12 @@ impl CbcSolver { impl Solver for CbcSolver { type Settings = CbcSolverSettings; + fn name() -> &'static str { + "cbc" + } + fn features() -> &'static [SolverFeatures] { - &[ - SolverFeatures::AggregatedNode, - SolverFeatures::AggregatedNodeFactors, - SolverFeatures::VirtualStorage, - ] + &[SolverFeatures::AggregatedNode, SolverFeatures::VirtualStorage] } fn setup(model: &Network, _settings: &Self::Settings) -> Result, PywrError> { @@ -240,10 +240,10 @@ impl Solver for CbcSolver { self.cbc.change_row_lower(self.builder.row_lower()); self.cbc.change_row_upper(self.builder.row_upper()); - for (row, column, coefficient) in self.builder.coefficients_to_update() { - // TODO raise an error for missing feature - // self.cbc.modify_coefficient(*row, *column, *coefficient) - } + // TODO raise an error for missing feature + // for (row, column, coefficient) in self.builder.coefficients_to_update() { + // self.cbc.modify_coefficient(*row, *column, *coefficient) + // } timings.update_constraints += now.elapsed(); diff --git a/pywr-core/src/solvers/clp/mod.rs b/pywr-core/src/solvers/clp/mod.rs index 93f67dd5..5f659503 100644 --- a/pywr-core/src/solvers/clp/mod.rs +++ b/pywr-core/src/solvers/clp/mod.rs @@ -229,6 +229,10 @@ impl ClpSolver { impl Solver for ClpSolver { type Settings = ClpSolverSettings; + fn name() -> &'static str { + "clp" + } + fn features() -> &'static [SolverFeatures] { &[ SolverFeatures::AggregatedNode, diff --git a/pywr-core/src/solvers/mod.rs b/pywr-core/src/solvers/mod.rs index c2c192a8..11d6ccaa 100644 --- a/pywr-core/src/solvers/mod.rs +++ b/pywr-core/src/solvers/mod.rs @@ -84,6 +84,7 @@ pub trait SolverSettings { pub trait Solver: Send { type Settings; + fn name() -> &'static str; /// An array of features that this solver provides. fn features() -> &'static [SolverFeatures]; fn setup(model: &Network, settings: &Self::Settings) -> Result, PywrError>; diff --git a/pywr-core/src/test_utils.rs b/pywr-core/src/test_utils.rs index dda2d43b..71ddd9c6 100644 --- a/pywr-core/src/test_utils.rs +++ b/pywr-core/src/test_utils.rs @@ -11,11 +11,11 @@ use crate::scenario::ScenarioGroupCollection; use crate::solvers::CbcSolver; #[cfg(feature = "ipm-ocl")] use crate::solvers::ClIpmF64Solver; -use crate::solvers::ClpSolver; #[cfg(feature = "highs")] use crate::solvers::HighsSolver; #[cfg(feature = "ipm-simd")] use crate::solvers::SimdIpmF64Solver; +use crate::solvers::{ClpSolver, Solver, SolverSettings}; use crate::timestep::{TimeDomain, TimestepDuration, Timestepper}; use crate::PywrError; use chrono::{Days, NaiveDate}; @@ -187,35 +187,21 @@ pub fn run_and_assert_parameter( ); model.network_mut().add_recorder(Box::new(rec)).unwrap(); - run_all_solvers(model) + run_all_solvers(model, &[]) } /// Run a model using each of the in-built solvers. /// /// The model will only be run if the solver has the required solver features (and /// is also enabled as a Cargo feature). -pub fn run_all_solvers(model: &Model) { - model - .run::(&Default::default()) - .expect("Failed to solve with CLP"); +pub fn run_all_solvers(model: &Model, solvers_without_features: &[&str]) { + check_features_and_run::(model, !solvers_without_features.contains(&"clp")); #[cfg(feature = "cbc")] - { - if model.check_solver_features::() { - model - .run::(&Default::default()) - .expect("Failed to solve with CBC"); - } - } + check_features_and_run::(model, !solvers_without_features.contains(&"cbc")); #[cfg(feature = "highs")] - { - if model.check_solver_features::() { - model - .run::(&Default::default()) - .expect("Failed to solve with Highs"); - } - } + check_features_and_run::(model, !solvers_without_features.contains(&"highs")); #[cfg(feature = "ipm-simd")] { @@ -236,6 +222,23 @@ pub fn run_all_solvers(model: &Model) { } } +/// Check features and +fn check_features_and_run(model: &Model, expect_features: bool) +where + S: Solver, + ::Settings: SolverSettings + Default, +{ + let has_features = model.check_solver_features::(); + if expect_features { + assert!(has_features); + model + .run::(&Default::default()) + .expect(&format!("Failed to solve with: {}", S::name())); + } else { + assert!(!has_features); + } +} + /// Make a simple system with random inputs. fn make_simple_system( network: &mut Network, diff --git a/pywr-core/src/virtual_storage.rs b/pywr-core/src/virtual_storage.rs index 9c096fae..c9e19dd3 100644 --- a/pywr-core/src/virtual_storage.rs +++ b/pywr-core/src/virtual_storage.rs @@ -414,7 +414,7 @@ mod tests { let domain = default_timestepper().try_into().unwrap(); let model = Model::new(domain, network); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } #[test] @@ -441,7 +441,7 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } #[test] @@ -481,6 +481,6 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } } From 1084ffada48e0fd0dcd08d1318b14597f6e0c5db Mon Sep 17 00:00:00 2001 From: James Tomlinson Date: Tue, 2 Jul 2024 15:12:32 +0100 Subject: [PATCH 3/5] fix: Implement Solver::name for Highs --- pywr-core/src/solvers/highs/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pywr-core/src/solvers/highs/mod.rs b/pywr-core/src/solvers/highs/mod.rs index 3eef0a5b..d52a9b9b 100644 --- a/pywr-core/src/solvers/highs/mod.rs +++ b/pywr-core/src/solvers/highs/mod.rs @@ -162,6 +162,10 @@ pub struct HighsSolver { impl Solver for HighsSolver { type Settings = HighsSolverSettings; + fn name() -> &'static str { + "highs" + } + fn features() -> &'static [SolverFeatures] { &[] } From bedbcb192b7e09b21558e81010fea00e25e01ea5 Mon Sep 17 00:00:00 2001 From: James Tomlinson Date: Tue, 2 Jul 2024 15:36:40 +0100 Subject: [PATCH 4/5] fix: Fix expected tests for highs and cbc solvers. --- pywr-core/src/aggregated_node.rs | 2 +- pywr-core/src/test_utils.rs | 12 ++++++++++-- pywr-core/src/virtual_storage.rs | 6 +++--- pywr-schema/src/model.rs | 3 +-- pywr-schema/src/nodes/delay.rs | 2 +- pywr-schema/src/nodes/piecewise_link.rs | 2 +- pywr-schema/src/nodes/piecewise_storage.rs | 4 ++-- pywr-schema/src/nodes/river_gauge.rs | 2 +- pywr-schema/src/nodes/river_split_with_gauge.rs | 2 +- pywr-schema/src/nodes/rolling_virtual_storage.rs | 2 +- pywr-schema/src/nodes/water_treatment_works.rs | 2 +- pywr-schema/src/timeseries/mod.rs | 2 +- 12 files changed, 24 insertions(+), 17 deletions(-) diff --git a/pywr-core/src/aggregated_node.rs b/pywr-core/src/aggregated_node.rs index 974f1697..6e969390 100644 --- a/pywr-core/src/aggregated_node.rs +++ b/pywr-core/src/aggregated_node.rs @@ -347,6 +347,6 @@ mod tests { let model = Model::new(default_time_domain().into(), network); - run_all_solvers(&model, &["cbc"]); + run_all_solvers(&model, &["cbc", "highs"]); } } diff --git a/pywr-core/src/test_utils.rs b/pywr-core/src/test_utils.rs index 71ddd9c6..698f2c18 100644 --- a/pywr-core/src/test_utils.rs +++ b/pywr-core/src/test_utils.rs @@ -230,12 +230,20 @@ where { let has_features = model.check_solver_features::(); if expect_features { - assert!(has_features); + assert!( + has_features, + "Solver `{}` was expected to have the required features", + S::name() + ); model .run::(&Default::default()) .expect(&format!("Failed to solve with: {}", S::name())); } else { - assert!(!has_features); + assert!( + !has_features, + "Solver `{}` was not expected to have the required features", + S::name() + ); } } diff --git a/pywr-core/src/virtual_storage.rs b/pywr-core/src/virtual_storage.rs index c9e19dd3..ba972dc2 100644 --- a/pywr-core/src/virtual_storage.rs +++ b/pywr-core/src/virtual_storage.rs @@ -414,7 +414,7 @@ mod tests { let domain = default_timestepper().try_into().unwrap(); let model = Model::new(domain, network); // Test all solvers - run_all_solvers(&model, &[]); + run_all_solvers(&model, &["highs"]); } #[test] @@ -441,7 +441,7 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model, &[]); + run_all_solvers(&model, &["highs"]); } #[test] @@ -481,6 +481,6 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model, &[]); + run_all_solvers(&model, &["highs"]); } } diff --git a/pywr-schema/src/model.rs b/pywr-schema/src/model.rs index 344f8841..84fa6b0c 100644 --- a/pywr-schema/src/model.rs +++ b/pywr-schema/src/model.rs @@ -1036,7 +1036,6 @@ mod core_tests { use ndarray::{Array1, Array2, Axis}; use pywr_core::{metric::MetricF64, recorders::AssertionRecorder, solvers::ClpSolver, test_utils::run_all_solvers}; use std::path::PathBuf; - use std::str::FromStr; fn model_str() -> &'static str { include_str!("./test_models/simple1.json") @@ -1067,7 +1066,7 @@ mod core_tests { network.add_recorder(Box::new(rec)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } /// Test that a cycle in parameter dependencies does not load. diff --git a/pywr-schema/src/nodes/delay.rs b/pywr-schema/src/nodes/delay.rs index 45cbea2d..93ea928a 100644 --- a/pywr-schema/src/nodes/delay.rs +++ b/pywr-schema/src/nodes/delay.rs @@ -197,6 +197,6 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } } diff --git a/pywr-schema/src/nodes/piecewise_link.rs b/pywr-schema/src/nodes/piecewise_link.rs index 103dd1d1..631ab3c5 100644 --- a/pywr-schema/src/nodes/piecewise_link.rs +++ b/pywr-schema/src/nodes/piecewise_link.rs @@ -223,6 +223,6 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } } diff --git a/pywr-schema/src/nodes/piecewise_storage.rs b/pywr-schema/src/nodes/piecewise_storage.rs index 189f0e48..6c18bebb 100644 --- a/pywr-schema/src/nodes/piecewise_storage.rs +++ b/pywr-schema/src/nodes/piecewise_storage.rs @@ -286,7 +286,7 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } /// Test running `piecewise_storage2.json` @@ -361,6 +361,6 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); } } diff --git a/pywr-schema/src/nodes/river_gauge.rs b/pywr-schema/src/nodes/river_gauge.rs index f5149a30..ec4ba87c 100644 --- a/pywr-schema/src/nodes/river_gauge.rs +++ b/pywr-schema/src/nodes/river_gauge.rs @@ -179,7 +179,7 @@ mod tests { assert_eq!(network.edges().len(), 6); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); // TODO assert the results! } diff --git a/pywr-schema/src/nodes/river_split_with_gauge.rs b/pywr-schema/src/nodes/river_split_with_gauge.rs index b0827deb..a1f43e0d 100644 --- a/pywr-schema/src/nodes/river_split_with_gauge.rs +++ b/pywr-schema/src/nodes/river_split_with_gauge.rs @@ -271,7 +271,7 @@ mod tests { assert_eq!(network.edges().len(), 6); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &[]); // TODO assert the results! } diff --git a/pywr-schema/src/nodes/rolling_virtual_storage.rs b/pywr-schema/src/nodes/rolling_virtual_storage.rs index 3d192e2f..eee11099 100644 --- a/pywr-schema/src/nodes/rolling_virtual_storage.rs +++ b/pywr-schema/src/nodes/rolling_virtual_storage.rs @@ -275,6 +275,6 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &["highs"]); } } diff --git a/pywr-schema/src/nodes/water_treatment_works.rs b/pywr-schema/src/nodes/water_treatment_works.rs index d2815ec6..d96f7728 100644 --- a/pywr-schema/src/nodes/water_treatment_works.rs +++ b/pywr-schema/src/nodes/water_treatment_works.rs @@ -408,6 +408,6 @@ mod tests { network.add_recorder(Box::new(recorder)).unwrap(); // Test all solvers - run_all_solvers(&model); + run_all_solvers(&model, &["cbc", "highs"]); } } diff --git a/pywr-schema/src/timeseries/mod.rs b/pywr-schema/src/timeseries/mod.rs index 78b78d32..e57cf936 100644 --- a/pywr-schema/src/timeseries/mod.rs +++ b/pywr-schema/src/timeseries/mod.rs @@ -311,6 +311,6 @@ mod tests { let recorder = AssertionRecorder::new("output-flow", MetricF64::NodeInFlow(idx), expected.clone(), None, None); model.network_mut().add_recorder(Box::new(recorder)).unwrap(); - run_all_solvers(&model) + run_all_solvers(&model, &[]) } } From 14cb35930ddcd8e4c9c3004aff18bd275956402d Mon Sep 17 00:00:00 2001 From: James Tomlinson Date: Sat, 6 Jul 2024 22:06:32 +0100 Subject: [PATCH 5/5] fix: Correct some comment typos. --- pywr-core/src/solvers/cbc/mod.rs | 2 +- pywr-core/src/solvers/cbc/settings.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pywr-core/src/solvers/cbc/mod.rs b/pywr-core/src/solvers/cbc/mod.rs index 7188d8fc..802f3d05 100644 --- a/pywr-core/src/solvers/cbc/mod.rs +++ b/pywr-core/src/solvers/cbc/mod.rs @@ -119,7 +119,7 @@ impl Cbc { let coefs = &elements[start as usize..end as usize]; unsafe { - let c_name = CString::new("row").expect("Failed to create CString for column name."); + let c_name = CString::new("row").expect("Failed to create CString for row name."); let sense = 'E'; Cbc_addRow( self.ptr, diff --git a/pywr-core/src/solvers/cbc/settings.rs b/pywr-core/src/solvers/cbc/settings.rs index 195663dd..2c065d19 100644 --- a/pywr-core/src/solvers/cbc/settings.rs +++ b/pywr-core/src/solvers/cbc/settings.rs @@ -1,6 +1,6 @@ use crate::solvers::SolverSettings; -/// Settings for the OpenCL IPM solvers. +/// Settings for the CBC solver. /// /// Create new settings using [`CbcSolverSettingsBuilder`] or use the default implementation; #[derive(PartialEq, Debug, Copy, Clone)]