From 98b74e81813ee746718ae1bfe75deecffd37ebe5 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Thu, 12 Sep 2024 22:02:28 +0200 Subject: [PATCH 1/5] clippy fixes --- sprs-ldl/src/lib.rs | 2 +- sprs/src/sparse/binop.rs | 2 ++ sprs/src/sparse/csmat.rs | 6 +++--- sprs/src/sparse/indptr.rs | 2 +- sprs/src/sparse/linalg/bicgstab.rs | 14 +++++++------- sprs/src/sparse/linalg/ordering.rs | 4 +++- sprs/src/sparse/smmp.rs | 7 ++++--- sprs/src/sparse/vec.rs | 12 ++++++------ .../sprs_suitesparse_camd/src/lib.rs | 6 +++++- 9 files changed, 32 insertions(+), 23 deletions(-) diff --git a/sprs-ldl/src/lib.rs b/sprs-ldl/src/lib.rs index 880f4e81..1a309f08 100644 --- a/sprs-ldl/src/lib.rs +++ b/sprs-ldl/src/lib.rs @@ -497,7 +497,7 @@ pub fn ldl_symbolic( /// Perform numeric LDLT decomposition /// -/// `pattern_workspace` is a [`DStack`](DStack) of capacity n +/// `pattern_workspace` is a [`DStack`] of capacity n #[allow(clippy::too_many_arguments)] pub fn ldl_numeric( mat: CsMatViewI, diff --git a/sprs/src/sparse/binop.rs b/sprs/src/sparse/binop.rs index cec20325..51e3305b 100644 --- a/sprs/src/sparse/binop.rs +++ b/sprs/src/sparse/binop.rs @@ -162,6 +162,8 @@ sparse_scalar_mul!(usize); sparse_scalar_mul!(f32); sparse_scalar_mul!(f64); +/// Apply binary operation to two sparse matrices +/// /// Applies a binary operation to matching non-zero elements /// of two sparse matrices. When e.g. only the `lhs` has a non-zero at a /// given location, `0` is inferred for the non-zero value of the other matrix. diff --git a/sprs/src/sparse/csmat.rs b/sprs/src/sparse/csmat.rs index 17b84a49..d71ddf85 100644 --- a/sprs/src/sparse/csmat.rs +++ b/sprs/src/sparse/csmat.rs @@ -1170,9 +1170,9 @@ where pub fn outer_iterator_papt<'a, 'perm: 'a>( &'a self, perm: PermViewI<'perm, I>, - ) -> impl std::iter::DoubleEndedIterator)> - + std::iter::ExactSizeIterator)> - + '_ { + ) -> impl std::iter::DoubleEndedIterator)> + + std::iter::ExactSizeIterator)> + + 'a { (0..self.outer_dims()).map(move |outer_ind| { let outer_ind_perm = perm.at(outer_ind); let range = self.indptr.outer_inds_sz(outer_ind_perm); diff --git a/sprs/src/sparse/indptr.rs b/sprs/src/sparse/indptr.rs index 96c975db..01dd6514 100644 --- a/sprs/src/sparse/indptr.rs +++ b/sprs/src/sparse/indptr.rs @@ -54,7 +54,7 @@ where .last() .copied() .map(Iptr::index_unchecked) - .map_or(false, |i| i > usize::max_value() / 2) + .map_or(false, |i| i > usize::MAX / 2) { // We do not allow indptr values to be larger than half // the maximum value of an usize, as that would clearly exhaust diff --git a/sprs/src/sparse/linalg/bicgstab.rs b/sprs/src/sparse/linalg/bicgstab.rs index 567871cf..4d74805c 100644 --- a/sprs/src/sparse/linalg/bicgstab.rs +++ b/sprs/src/sparse/linalg/bicgstab.rs @@ -74,18 +74,18 @@ //! * Hard restart to check true error before claiming convergence //! * Soft-restart logic uses a correct metric of perpendicularity instead of a magnitude heuristic //! * The usual method, which compares a fixed value to `rho`, does not capture the fact that the -//! magnitude of `rho` will naturally decrease as the solver approaches convergence +//! magnitude of `rho` will naturally decrease as the solver approaches convergence //! * This change eliminates the effect where the a soft restart is performed on every iteration for the last few -//! iterations of any solve with a reasonable error tolerance +//! iterations of any solve with a reasonable error tolerance //! * Hard-restart logic provides some real guarantee of correctness //! * The usual implementations keep a cheap, but inaccurate, running estimate of the error //! * That decreases the cost of iterations by about half by eliminating a matrix-vector multiplication, -//! but allows the estimate of error to drift numerically, which causes the solver to return claiming -//! convergence when the solved output does not, in fact, match the input system +//! but allows the estimate of error to drift numerically, which causes the solver to return claiming +//! convergence when the solved output does not, in fact, match the input system //! * This change guarantees that the solver will not return claiming convergence unless the solution -//! actually matches the input system, and will refresh its estimate of the error and continue iterations -//! if it has reached a falsely-converged state, continuing until it either reaches true convergence or -//! reaches maximum iterations +//! actually matches the input system, and will refresh its estimate of the error and continue iterations +//! if it has reached a falsely-converged state, continuing until it either reaches true convergence or +//! reaches maximum iterations use crate::indexing::SpIndex; use crate::sparse::{CsMatViewI, CsVecI, CsVecViewI}; use num_traits::One; diff --git a/sprs/src/sparse/linalg/ordering.rs b/sprs/src/sparse/linalg/ordering.rs index 0725f4c3..230f3c57 100644 --- a/sprs/src/sparse/linalg/ordering.rs +++ b/sprs/src/sparse/linalg/ordering.rs @@ -272,6 +272,8 @@ pub mod order { use crate::indexing::SpIndex; use crate::sparse::permutation::PermOwnedI; + /// Internal trait for working with Cuthill-McKee + /// /// This trait is very deeply integrated with the inner workings of the /// Cuthill-McKee algorithm implemented here. It is conceptually only an /// enum, specifying if the Cuthill-McKee ordering should be built in @@ -434,7 +436,7 @@ pub mod order { /// - `starting_strategy` - The strategy to use for choosing a starting vertex. /// /// - `directed_ordering` - The order of the computed ordering, should either be -/// `Forward` or `Reverse`. +/// `Forward` or `Reverse`. pub fn cuthill_mckee_custom( mat: CsMatViewI, mut starting_strategy: S, diff --git a/sprs/src/sparse/smmp.rs b/sprs/src/sparse/smmp.rs index bf5c3450..a61cbfac 100644 --- a/sprs/src/sparse/smmp.rs +++ b/sprs/src/sparse/smmp.rs @@ -32,9 +32,10 @@ pub enum ThreadingStrategy { } #[cfg(feature = "multi_thread")] -thread_local!(static THREADING_STRAT: RefCell = - RefCell::new(ThreadingStrategy::Automatic) -); +thread_local! { + static THREADING_STRAT: RefCell = + const { RefCell::new(ThreadingStrategy::Automatic) }; +} /// Set the threading strategy for matrix products in this thread. /// diff --git a/sprs/src/sparse/vec.rs b/sprs/src/sparse/vec.rs index 94c09db6..c014979d 100644 --- a/sprs/src/sparse/vec.rs +++ b/sprs/src/sparse/vec.rs @@ -296,8 +296,9 @@ where // context to a plain function, which enables specifying the type // Needless to say, this needs to go when it's no longer necessary #[inline(always)] - fn hack_instead_of_closure(vi: (&V, usize)) -> (usize, &N) + fn hack_instead_of_closure(vi: (&V, usize)) -> (usize, &N) where + V: ?Sized, V: DenseVector, { (vi.1, vi.0.index(vi.1)) @@ -824,10 +825,11 @@ where /// assert_eq!(4., v1.dot(&v1)); /// assert_eq!(16., v2.dot(&v2)); /// ``` - pub fn dot<'b, T: IntoSparseVecIter<'b, N>>(&'b self, rhs: T) -> N + pub fn dot<'b, T>(&'b self, rhs: T) -> N where N: 'b + crate::MulAcc + num_traits::Zero, I: 'b, + T: IntoSparseVecIter<'b, N>, >::IterType: Iterator, T: Copy, // T is supposed to be a reference type @@ -841,13 +843,11 @@ where /// that can be interpreted as a sparse vector (hence sparse vectors, std /// vectors and slices, and ndarray's dense vectors work). /// The output type can be any type supporting `MulAcc`. - pub fn dot_acc<'b, T: IntoSparseVecIter<'b, M>, M, Acc>( - &'b self, - rhs: T, - ) -> Acc + pub fn dot_acc<'b, T, M, Acc>(&'b self, rhs: T) -> Acc where Acc: 'b + crate::MulAcc + num_traits::Zero, M: 'b, + T: IntoSparseVecIter<'b, M>, >::IterType: Iterator, T: Copy, // T is supposed to be a reference type diff --git a/suitesparse_bindings/sprs_suitesparse_camd/src/lib.rs b/suitesparse_bindings/sprs_suitesparse_camd/src/lib.rs index 4cbec47f..3e7b70fe 100644 --- a/suitesparse_bindings/sprs_suitesparse_camd/src/lib.rs +++ b/suitesparse_bindings/sprs_suitesparse_camd/src/lib.rs @@ -5,8 +5,10 @@ use suitesparse_camd_sys::*; // FIXME should be using SuiteSparseInt::MAX but this will not compile // in rust 1.42 as u32::MAX was introduced in 1.43. This can be changed if // the MSRV is bumped. -const MAX_INT32: usize = std::u32::MAX as usize; +const MAX_INT32: usize = u32::MAX as usize; +/// Find permutation of `mat` +/// /// Find a permutation matrix P which reduces the fill-in of the square /// sparse matrix `mat` in Cholesky factorization (ie, the number of nonzeros /// of the Cholesky factorization of P A P^T is less than for the Cholesky @@ -77,6 +79,8 @@ where Ok(PermOwnedI::new(perm)) } +/// Find permutation of `mat` +/// /// Find a permutation matrix P which reduces the fill-in of the square /// sparse matrix `mat` in Cholesky factorization (ie, the number of nonzeros /// of the Cholesky factorization of P A P^T is less than for the Cholesky From edad4269b5b3dabd116dbb9bfeaacdb4b5d6f3b0 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Thu, 12 Sep 2024 22:03:13 +0200 Subject: [PATCH 2/5] Remove unused trait --- sprs/src/sparse/compressed.rs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/sprs/src/sparse/compressed.rs b/sprs/src/sparse/compressed.rs index 41d4609c..36d334dc 100644 --- a/sprs/src/sparse/compressed.rs +++ b/sprs/src/sparse/compressed.rs @@ -30,22 +30,3 @@ where self.transpose_view() } } - -/// The `SpVecView` trait describes types that can be seen as a view into -/// a `CsVec` -pub trait SpVecView { - /// Return a view into the current vector - fn view(&self) -> CsVecViewI; -} - -impl SpVecView - for CsVecBase -where - IndStorage: Deref, - DataStorage: Deref, - I: SpIndex, -{ - fn view(&self) -> CsVecViewI { - self.view() - } -} From 930d7f40a3b00f101f2ec4ce61de8ad0aaa3184a Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Thu, 12 Sep 2024 22:09:31 +0200 Subject: [PATCH 3/5] Bump MSRV --- .github/workflows/ci.yml | 2 +- changelog.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c02d4752..d4310a24 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -66,7 +66,7 @@ jobs: include: - build: msrv os: ubuntu-20.04 - rust: 1.64.0 + rust: 1.77.0 - build: stable os: ubuntu-20.04 rust: stable diff --git a/changelog.rst b/changelog.rst index beccade9..1ad968ae 100644 --- a/changelog.rst +++ b/changelog.rst @@ -4,7 +4,7 @@ Changelog - Unreleased - Fixed a compilation regression in csmat_binop - - Bump MSRV (1.64) + - Bump MSRV (1.77) - add support for reading/writing Complex{32,64} matrices in matrixmarket format. Also change semantics so that files of kind {integer,real,complex} are only readable into matrices of the same kind (so integer can be read into [iu]{8,16,32,64,size}), From a0a00e6c498115f279572f29c9e97fdd368ee0f0 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Thu, 12 Sep 2024 22:26:44 +0200 Subject: [PATCH 4/5] More clippy --- sprs/src/sparse/csmat.rs | 18 +++++++++--------- sprs/src/sparse/linalg/trisolve.rs | 2 +- sprs/src/sparse/vec.rs | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/sprs/src/sparse/csmat.rs b/sprs/src/sparse/csmat.rs index d71ddf85..94a2b783 100644 --- a/sprs/src/sparse/csmat.rs +++ b/sprs/src/sparse/csmat.rs @@ -841,14 +841,14 @@ where nnz / (rows * cols) } - /// Number of outer dimensions, that ie equal to self.rows() for a CSR - /// matrix, and equal to self.cols() for a CSC matrix + /// Number of outer dimensions, that ie equal to `self.rows()` for a CSR + /// matrix, and equal to `self.cols()` for a CSC matrix pub fn outer_dims(&self) -> usize { outer_dimension(self.storage, self.nrows, self.ncols) } - /// Number of inner dimensions, that ie equal to self.cols() for a CSR - /// matrix, and equal to self.rows() for a CSC matrix + /// Number of inner dimensions, that ie equal to `self.cols()` for a CSR + /// matrix, and equal to `self.rows()` for a CSC matrix pub fn inner_dims(&self) -> usize { match self.storage { CSC => self.nrows, @@ -870,10 +870,10 @@ where } } - /// The array of offsets in the indices() and data() slices. + /// The array of offsets in the `indices()` `and data()` slices. /// The elements of the slice at outer dimension i - /// are available between the elements indptr\[i\] and indptr\[i+1\] - /// in the indices() and data() slices. + /// are available between the elements `indptr\[i\]` and `indptr\[i+1\]` + /// in the `indices()` and `data()` slices. /// /// # Example /// @@ -921,12 +921,12 @@ where } /// The inner dimension location for each non-zero value. See - /// the documentation of indptr() for more explanations. + /// the documentation of `indptr()` for more explanations. pub fn indices(&self) -> &[I] { &self.indices[..] } - /// The non-zero values. See the documentation of indptr() + /// The non-zero values. See the documentation of `indptr()` /// for more explanations. pub fn data(&self) -> &[N] { &self.data[..] diff --git a/sprs/src/sparse/linalg/trisolve.rs b/sprs/src/sparse/linalg/trisolve.rs index 99bdf3c7..9e5c3675 100644 --- a/sprs/src/sparse/linalg/trisolve.rs +++ b/sprs/src/sparse/linalg/trisolve.rs @@ -279,7 +279,7 @@ where /// /// # Panics /// -/// * if dstack.capacity() is too small +/// * if `dstack.capacity()` is too small /// * if dstack is not empty /// * if `w_workspace` is not of length n /// diff --git a/sprs/src/sparse/vec.rs b/sprs/src/sparse/vec.rs index c014979d..76fb272e 100644 --- a/sprs/src/sparse/vec.rs +++ b/sprs/src/sparse/vec.rs @@ -689,7 +689,7 @@ where /// Check the sparse structure, namely that: /// - indices are sorted - /// - all indices are less than dims() + /// - all indices are less than `dims()` pub fn check_structure(&self) -> Result<(), StructureError> { // Make sure indices can be converted to usize for i in self.indices.iter() { From 9a2ffd02d96263beb32bd04e8a4f2ba49d4302e5 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Thu, 12 Sep 2024 22:41:25 +0200 Subject: [PATCH 5/5] Fix ldll building --- suitesparse_bindings/suitesparse-src/build.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/suitesparse_bindings/suitesparse-src/build.rs b/suitesparse_bindings/suitesparse-src/build.rs index 5ea70a69..26e77935 100644 --- a/suitesparse_bindings/suitesparse-src/build.rs +++ b/suitesparse_bindings/suitesparse-src/build.rs @@ -44,26 +44,21 @@ fn main() { if std::env::var_os("CARGO_FEATURE_LDL").is_some() { // We first build ldl with LDL_LONG to make the bindings to // the long bits of the library - cc::Build::new() + let ldll_artifact = cc::Build::new() .include("SuiteSparse/SuiteSparse_config") .include("SuiteSparse/LDL/Include") .file("SuiteSparse/LDL/Source/ldl.c") .cargo_metadata(false) .define("LDL_LONG", None) - .compile("ldll"); - // We must then copy this to another location since the next - // invocation is just a compile definition - let mut ldl_path = std::path::PathBuf::from(root.clone()); - ldl_path.push("SuiteSparse/LDL/Source/ldl.o"); - let mut ldll_path = ldl_path.clone(); - ldll_path.set_file_name("ldll.o"); - std::fs::copy(&ldl_path, &ldll_path).unwrap(); + .compile_intermediates(); + let (ldll_artifact, rest) = ldll_artifact.split_first().unwrap(); + assert!(rest.is_empty()); // And now we build ldl again (in int form), and link with the long bits cc::Build::new() .include("SuiteSparse/SuiteSparse_config") .include("SuiteSparse/LDL/Include") .file("SuiteSparse/LDL/Source/ldl.c") - .object(&ldll_path) + .object(ldll_artifact) .cargo_metadata(false) .compile("ldl"); }