feat: Unify solver features and fix Clippy issues. (#232)

Fix several issues regarding features that Clippy raised. Extends Clippy coverage to all crates except `ipm-simd`, which only works on nightly. Unifies the solver features to `pywr-cli`, `pywr-schema` and `pywr-python` crates. These enable their dependent crate features as required.
pywr · Jul 29, 2024 · 3ed4f90 · 3ed4f90
1 parent 39ae3a9
commit 3ed4f90
Show file tree

Hide file tree

Showing 8 changed files with 37 additions and 30 deletions.
diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml
@@ -20,7 +20,7 @@ jobs:
         with:
           submodules: true
       - name: Run Clippy
-        run: cargo clippy --all-targets --features highs,cbc
+        run: cargo clippy --all-targets --features highs,cbc --all --exclude ipm-simd
       - name: Install latest mdbook
         run: |
           tag=$(curl 'https://api.github.com/repos/rust-lang/mdbook/releases/latest' | jq -r '.tag_name')

diff --git a/ipm-ocl/src/lib.rs b/ipm-ocl/src/lib.rs
@@ -108,26 +108,14 @@ where
         let row_offsets = ocl::Buffer::<u32>::builder()
             .queue(queue.clone())
             .flags(ocl::flags::MEM_READ_WRITE)
-            .copy_host_slice(
-                a.row_offsets()
-                    .into_iter()
-                    .map(|r| *r as u32)
-                    .collect::<Vec<_>>()
-                    .as_slice(),
-            )
+            .copy_host_slice(a.row_offsets().iter().map(|r| *r as u32).collect::<Vec<_>>().as_slice())
             .len(a.row_offsets().len())
             .build()?;
 
         let col_indices = ocl::Buffer::<u32>::builder()
             .queue(queue.clone())
             .flags(ocl::flags::MEM_READ_WRITE)
-            .copy_host_slice(
-                a.col_indices()
-                    .into_iter()
-                    .map(|r| *r as u32)
-                    .collect::<Vec<_>>()
-                    .as_slice(),
-            )
+            .copy_host_slice(a.col_indices().iter().map(|r| *r as u32).collect::<Vec<_>>().as_slice())
             .len(a.col_indices().len())
             .build()?;
 
@@ -439,6 +427,7 @@ impl<T> PathFollowingDirectClSolver<T>
 where
     T: ocl::OclPrm + GetClProgram,
 {
+    #[allow(clippy::too_many_arguments)]
     pub fn from_data(
         queue: &ocl::Queue,
         program: &ocl::Program,
@@ -456,7 +445,7 @@ where
         let buffers = PathFollowingDirectClBuffers::from_data(&a, num_lps, queue)?;
 
         let kernel_normal_init = ocl::Kernel::builder()
-            .program(&program)
+            .program(program)
             .name("normal_eqn_init")
             .queue(queue.clone())
             .global_work_size(num_lps)
@@ -470,7 +459,7 @@ where
             .build()?;
 
         let kernel_normal_eq_step = ocl::Kernel::builder()
-            .program(&program)
+            .program(program)
             .name("normal_eqn_step")
             .queue(queue.clone())
             .global_work_size(num_lps)
@@ -619,6 +608,6 @@ mod tests {
         let queue = ocl::Queue::new(&context, device, None).unwrap();
 
         let a = test_matrx();
-        let pf = PathFollowingDirectClBuffers::from_data(&a, 10, &queue).unwrap();
+        let _pf = PathFollowingDirectClBuffers::from_data(&a, 10, &queue).unwrap();
     }
 }
diff --git a/pywr-cli/Cargo.toml b/pywr-cli/Cargo.toml
@@ -26,3 +26,9 @@ schemars = { workspace = true }
 
 pywr-core = { path = "../pywr-core" }
 pywr-schema = { path = "../pywr-schema" }
+
+[features]
+cbc = ["pywr-core/cbc", "pywr-schema/cbc"]
+highs = ["pywr-core/highs", "pywr-schema/highs"]
+ipm-ocl = ["pywr-core/ipm-ocl", "pywr-schema/ipm-ocl"]
+ipm-simd = ["pywr-core/ipm-simd", "pywr-schema/ipm-simd"]
diff --git a/pywr-cli/src/main.rs b/pywr-cli/src/main.rs
@@ -24,7 +24,7 @@ use std::path::{Path, PathBuf};
 enum Solver {
     Clp,
     #[cfg(feature = "highs")]
-    HIGHS,
+    Highs,
     #[cfg(feature = "ipm-ocl")]
     CLIPMF32,
     #[cfg(feature = "ipm-ocl")]
@@ -38,7 +38,7 @@ impl Display for Solver {
         match self {
             Solver::Clp => write!(f, "clp"),
             #[cfg(feature = "highs")]
-            Solver::HIGHS => write!(f, "highs"),
+            Solver::Highs => write!(f, "highs"),
             #[cfg(feature = "ipm-ocl")]
             Solver::CLIPMF32 => write!(f, "clipmf32"),
             #[cfg(feature = "ipm-ocl")]
@@ -265,7 +265,7 @@ fn run(path: &Path, solver: &Solver, data_path: Option<&Path>, output_path: Opti
     match *solver {
         Solver::Clp => model.run::<ClpSolver>(&ClpSolverSettings::default()),
         #[cfg(feature = "highs")]
-        Solver::HIGHS => model.run::<HighsSolver>(&HighsSolverSettings::default()),
+        Solver::Highs => model.run::<HighsSolver>(&HighsSolverSettings::default()),
         #[cfg(feature = "ipm-ocl")]
         Solver::CLIPMF32 => model.run_multi_scenario::<ClIpmF32Solver>(&ClIpmSolverSettings::default()),
         #[cfg(feature = "ipm-ocl")]
@@ -287,7 +287,7 @@ fn run_multi(path: &Path, solver: &Solver, data_path: Option<&Path>, output_path
     match *solver {
         Solver::Clp => model.run::<ClpSolver>(&ClpSolverSettings::default()),
         #[cfg(feature = "highs")]
-        Solver::HIGHS => model.run::<HighsSolver>(&HighsSolverSettings::default()),
+        Solver::Highs => model.run::<HighsSolver>(&HighsSolverSettings::default()),
         #[cfg(feature = "ipm-ocl")]
         Solver::CLIPMF32 => model.run_multi_scenario::<ClIpmF32Solver>(&ClIpmSolverSettings::default()),
         #[cfg(feature = "ipm-ocl")]
@@ -305,7 +305,7 @@ fn run_random(num_systems: usize, density: usize, num_scenarios: usize, solver:
     match *solver {
         Solver::Clp => model.run::<ClpSolver>(&ClpSolverSettings::default()),
         #[cfg(feature = "highs")]
-        Solver::HIGHS => model.run::<HighsSolver>(&HighsSolverSettings::default()),
+        Solver::Highs => model.run::<HighsSolver>(&HighsSolverSettings::default()),
         #[cfg(feature = "ipm-ocl")]
         Solver::CLIPMF32 => model.run_multi_scenario::<ClIpmF32Solver>(&ClIpmSolverSettings::default()),
         #[cfg(feature = "ipm-ocl")]

diff --git a/pywr-core/src/network.rs b/pywr-core/src/network.rs
@@ -1648,8 +1648,6 @@ mod tests {
     use crate::parameters::{ActivationFunction, ControlCurveInterpolatedParameter, Parameter};
     use crate::recorders::AssertionRecorder;
     use crate::scenario::{ScenarioDomain, ScenarioGroupCollection, ScenarioIndex};
-    #[cfg(feature = "clipm")]
-    use crate::solvers::{ClIpmF64Solver, SimdIpmF64Solver};
     use crate::solvers::{ClpSolver, ClpSolverSettings};
     use crate::test_utils::{run_all_solvers, simple_model, simple_storage_model};
     use float_cmp::assert_approx_eq;

diff --git a/pywr-python/Cargo.toml b/pywr-python/Cargo.toml
@@ -30,3 +30,9 @@ pywr-schema = { path = "../pywr-schema" }
 [lib]
 name = "pywr"
 crate-type = ["cdylib"]
+
+[features]
+cbc = ["pywr-core/cbc", "pywr-schema/cbc"]
+highs = ["pywr-core/highs", "pywr-schema/highs"]
+ipm-ocl = ["pywr-core/ipm-ocl", "pywr-schema/ipm-ocl"]
+ipm-simd = ["pywr-core/ipm-simd", "pywr-schema/ipm-simd"]
diff --git a/pywr-python/src/lib.rs b/pywr-python/src/lib.rs
@@ -14,7 +14,7 @@ use pyo3::types::{PyDict, PyType};
 use pywr_core::solvers::{ClIpmF32Solver, ClIpmF64Solver, ClIpmSolverSettings};
 use pywr_core::solvers::{ClpSolver, ClpSolverSettings, ClpSolverSettingsBuilder};
 #[cfg(feature = "highs")]
-use pywr_core::solvers::{HighsSolver, HighsSolverSettings, HighsSolverSettings, HighsSolverSettingsBuilde};
+use pywr_core::solvers::{HighsSolver, HighsSolverSettings, HighsSolverSettingsBuilder};
 use pywr_schema::model::DateType;
 use std::fmt;
 use std::path::PathBuf;
@@ -118,12 +118,16 @@ impl Model {
             #[cfg(feature = "highs")]
             "highs" => {
                 let settings = build_highs_settings(solver_kwargs)?;
-                model.run::<HighsSolver>(&HighsSolverSettings::default())?;
+                self.model.run::<HighsSolver>(&settings)?;
             }
             #[cfg(feature = "ipm-ocl")]
-            "clipm-f32" => model.run_multi_scenario::<ClIpmF32Solver>(&ClIpmSolverSettings::default()),
+            "clipm-f32" => self
+                .model
+                .run_multi_scenario::<ClIpmF32Solver>(&ClIpmSolverSettings::default()),
             #[cfg(feature = "ipm-ocl")]
-            "clipm-f64" => model.run_multi_scenario::<ClIpmF64Solver>(&ClIpmSolverSettings::default()),
+            "clipm-f64" => self
+                .model
+                .run_multi_scenario::<ClIpmF64Solver>(&ClIpmSolverSettings::default()),
             _ => return Err(PyRuntimeError::new_err(format!("Unknown solver: {}", solver_name))),
         }
 
@@ -163,7 +167,7 @@ fn build_clp_settings(kwargs: Option<&Bound<'_, PyDict>>) -> PyResult<ClpSolverS
 }
 
 #[cfg(feature = "highs")]
-fn build_highs_settings(kwargs: Option<&PyDict>) -> PyResult<HighsSolverSettings> {
+fn build_highs_settings(kwargs: Option<&Bound<'_, PyDict>>) -> PyResult<HighsSolverSettings> {
     let mut builder = HighsSolverSettingsBuilder::default();
 
     if let Some(kwargs) = kwargs {

diff --git a/pywr-schema/Cargo.toml b/pywr-schema/Cargo.toml
@@ -42,3 +42,7 @@ tempfile = "3.3.0"
 # Core feature requires additional dependencies
 core = ["dep:pywr-core", "dep:hdf5", "dep:csv", "dep:polars", "dep:pyo3", "dep:pyo3-polars", "dep:ndarray", "dep:tracing"]
 default = ["core"]
+cbc = ["pywr-core/cbc"]
+highs = ["pywr-core/highs"]
+ipm-ocl = ["pywr-core/ipm-ocl"]
+ipm-simd = ["pywr-core/ipm-simd"]