From f9aca24b1567115e23d0968d227f0b2e7f1f97ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20L=C3=B6schner?= Date: Sun, 7 Nov 2021 21:32:11 +0100 Subject: [PATCH 01/20] Implement Serialize and Deserialize for CsrMatrix --- nalgebra-sparse/Cargo.toml | 5 ++- nalgebra-sparse/src/csr.rs | 64 ++++++++++++++++++++++++++++++++++ nalgebra-sparse/tests/serde.rs | 62 ++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 nalgebra-sparse/tests/serde.rs diff --git a/nalgebra-sparse/Cargo.toml b/nalgebra-sparse/Cargo.toml index 6f7a7b4a..eec7326d 100644 --- a/nalgebra-sparse/Cargo.toml +++ b/nalgebra-sparse/Cargo.toml @@ -15,6 +15,7 @@ license = "Apache-2.0" [features] proptest-support = ["proptest", "nalgebra/proptest-support"] compare = [ "matrixcompare-core" ] +serde-serialize = [ "serde/std" ] # Enable matrix market I/O io = [ "pest", "pest_derive" ] @@ -29,12 +30,14 @@ proptest = { version = "1.0", optional = true } matrixcompare-core = { version = "0.1.0", optional = true } pest = { version = "2", optional = true } pest_derive = { version = "2", optional = true } +serde = { version = "1.0", default-features = false, features = [ "derive" ], optional = true } [dev-dependencies] itertools = "0.10" matrixcompare = { version = "0.3.0", features = [ "proptest-support" ] } nalgebra = { version="0.30", path = "../", features = ["compare"] } +serde_json = "1.0" [package.metadata.docs.rs] # Enable certain features when building docs for docs.rs -features = [ "proptest-support", "compare" ] \ No newline at end of file +features = [ "proptest-support", "compare" ] diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 4324d18d..b98717b8 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -10,6 +10,9 @@ use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKin use nalgebra::Scalar; use num_traits::One; +#[cfg(feature = "serde-serialize")] +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + use std::iter::FromIterator; use std::slice::{Iter, IterMut}; @@ -596,6 +599,67 @@ impl CsrMatrix { } } +#[cfg_attr(feature = "serde-serialize", derive(Serialize))] +struct CsrMatrixSerializationHelper<'a, T> { + nrows: usize, + ncols: usize, + row_offsets: &'a [usize], + col_indices: &'a [usize], + values: &'a [T], +} + +#[cfg(feature = "serde-serialize")] +impl Serialize for CsrMatrix +where + T: Serialize, +{ + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CsrMatrixSerializationHelper { + nrows: self.nrows(), + ncols: self.ncols(), + row_offsets: self.row_offsets(), + col_indices: self.col_indices(), + values: self.values(), + } + .serialize(serializer) + } +} + +#[cfg_attr(feature = "serde-serialize", derive(Deserialize))] +struct CsrMatrixDeserializationHelper { + nrows: usize, + ncols: usize, + row_offsets: Vec, + col_indices: Vec, + values: Vec, +} + +#[cfg(feature = "serde-serialize")] +impl<'de, T> Deserialize<'de> for CsrMatrix +where + T: for<'de2> Deserialize<'de2>, +{ + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let CsrMatrixDeserializationHelper { + nrows, + ncols, + row_offsets, + col_indices, + values, + } = CsrMatrixDeserializationHelper::deserialize(deserializer)?; + CsrMatrix::try_from_csr_data(nrows, ncols, row_offsets, col_indices, values) + .map(|m| m.into()) + // TODO: More specific error + .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid CSR matrix"), &"a valid CSR matrix")) + } +} + /// Convert pattern format errors into more meaningful CSR-specific errors. /// /// This ensures that the terminology is consistent: we are talking about rows and columns, diff --git a/nalgebra-sparse/tests/serde.rs b/nalgebra-sparse/tests/serde.rs new file mode 100644 index 00000000..ecee76d1 --- /dev/null +++ b/nalgebra-sparse/tests/serde.rs @@ -0,0 +1,62 @@ +#![cfg(feature = "serde-serialize")] +//! Serialization tests +#[cfg(any(not(feature = "proptest-support"), not(feature = "compare")))] +compile_error!("Tests must be run with features `proptest-support` and `compare`"); + +#[macro_use] +pub mod common; + +use nalgebra_sparse::csr::CsrMatrix; + +use proptest::prelude::*; +use serde::{Deserialize, Serialize}; + +use crate::common::csr_strategy; + +fn json_roundtrip Deserialize<'a>>(csr: &CsrMatrix) -> CsrMatrix { + let serialized = serde_json::to_string(csr).unwrap(); + let deserialized: CsrMatrix = serde_json::from_str(&serialized).unwrap(); + deserialized +} + +#[test] +fn csr_roundtrip() { + { + // A CSR matrix with zero explicitly stored entries + let offsets = vec![0, 0, 0, 0]; + let indices = vec![]; + let values = Vec::::new(); + let matrix = CsrMatrix::try_from_csr_data(3, 2, offsets, indices, values).unwrap(); + + assert_eq!(json_roundtrip(&matrix), matrix); + } + + { + // An arbitrary CSR matrix + let offsets = vec![0, 2, 2, 5]; + let indices = vec![0, 5, 1, 2, 3]; + let values = vec![0, 1, 2, 3, 4]; + let matrix = + CsrMatrix::try_from_csr_data(3, 6, offsets.clone(), indices.clone(), values.clone()) + .unwrap(); + + assert_eq!(json_roundtrip(&matrix), matrix); + } +} + +#[test] +fn invalid_csr_deserialize() { + // Valid matrix: {"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]} + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,8,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3,1,1],"values":[0,1,2,3,4]}"#).is_err()); + // The following actually panics ('range end index 10 out of range for slice of length 5', nalgebra-sparse\src\pattern.rs:156:38) + //assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,10,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); +} + +proptest! { + #[test] + fn csr_roundtrip_proptest(csr in csr_strategy()) { + prop_assert_eq!(json_roundtrip(&csr), csr); + } +} From 18b694dad282c543cc2eb786f783f7b94afa3a0d Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Mon, 8 Nov 2021 11:10:58 +0100 Subject: [PATCH 02/20] Move serialization helper structs into trait impls --- nalgebra-sparse/src/csr.rs | 48 +++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index b98717b8..bc2a3df0 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -599,15 +599,6 @@ impl CsrMatrix { } } -#[cfg_attr(feature = "serde-serialize", derive(Serialize))] -struct CsrMatrixSerializationHelper<'a, T> { - nrows: usize, - ncols: usize, - row_offsets: &'a [usize], - col_indices: &'a [usize], - values: &'a [T], -} - #[cfg(feature = "serde-serialize")] impl Serialize for CsrMatrix where @@ -617,7 +608,16 @@ where where S: Serializer, { - CsrMatrixSerializationHelper { + #[derive(Serialize)] + struct CsrMatrixSerializationData<'a, T> { + nrows: usize, + ncols: usize, + row_offsets: &'a [usize], + col_indices: &'a [usize], + values: &'a [T], + } + + CsrMatrixSerializationData { nrows: self.nrows(), ncols: self.ncols(), row_offsets: self.row_offsets(), @@ -628,15 +628,6 @@ where } } -#[cfg_attr(feature = "serde-serialize", derive(Deserialize))] -struct CsrMatrixDeserializationHelper { - nrows: usize, - ncols: usize, - row_offsets: Vec, - col_indices: Vec, - values: Vec, -} - #[cfg(feature = "serde-serialize")] impl<'de, T> Deserialize<'de> for CsrMatrix where @@ -646,14 +637,17 @@ where where D: Deserializer<'de>, { - let CsrMatrixDeserializationHelper { - nrows, - ncols, - row_offsets, - col_indices, - values, - } = CsrMatrixDeserializationHelper::deserialize(deserializer)?; - CsrMatrix::try_from_csr_data(nrows, ncols, row_offsets, col_indices, values) + #[derive(Deserialize)] + struct CsrMatrixDeserializationData { + nrows: usize, + ncols: usize, + row_offsets: Vec, + col_indices: Vec, + values: Vec, + } + + let de = CsrMatrixDeserializationData::deserialize(deserializer)?; + CsrMatrix::try_from_csr_data(de.nrows, de.ncols, de.row_offsets, de.col_indices, de.values) .map(|m| m.into()) // TODO: More specific error .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid CSR matrix"), &"a valid CSR matrix")) From 40d8a904a3e5c1f486adfbc40fb2562254985c64 Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 10:30:02 +0100 Subject: [PATCH 03/20] Implement Serialize, Deserialize for Csc, Coo; move helper out of impls --- nalgebra-sparse/src/coo.rs | 60 ++++++++++++++++++ nalgebra-sparse/src/csc.rs | 59 ++++++++++++++++++ nalgebra-sparse/src/csr.rs | 40 ++++++------ nalgebra-sparse/tests/serde.rs | 110 +++++++++++++++++++++++++++++++-- 4 files changed, 245 insertions(+), 24 deletions(-) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index 34e5ceec..35a14083 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -2,6 +2,9 @@ use crate::SparseFormatError; +#[cfg(feature = "serde-serialize")] +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + /// A COO representation of a sparse matrix. /// /// A COO matrix stores entries in coordinate-form, that is triplets `(i, j, v)`, where `i` and `j` @@ -273,3 +276,60 @@ impl CooMatrix { (self.row_indices, self.col_indices, self.values) } } + +#[cfg(feature = "serde-serialize")] +#[derive(Serialize)] +struct CooMatrixSerializationData<'a, T> { + nrows: usize, + ncols: usize, + row_indices: &'a [usize], + col_indices: &'a [usize], + values: &'a [T], +} + +#[cfg(feature = "serde-serialize")] +impl Serialize for CooMatrix +where + T: Serialize, +{ + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CooMatrixSerializationData { + nrows: self.nrows(), + ncols: self.ncols(), + row_indices: self.row_indices(), + col_indices: self.col_indices(), + values: self.values(), + } + .serialize(serializer) + } +} + +#[cfg(feature = "serde-serialize")] +#[derive(Deserialize)] +struct CooMatrixDeserializationData { + nrows: usize, + ncols: usize, + row_indices: Vec, + col_indices: Vec, + values: Vec, +} + +#[cfg(feature = "serde-serialize")] +impl<'de, T> Deserialize<'de> for CooMatrix +where + T: for<'de2> Deserialize<'de2>, +{ + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let de = CooMatrixDeserializationData::deserialize(deserializer)?; + CooMatrix::try_from_triplets(de.nrows, de.ncols, de.row_indices, de.col_indices, de.values) + .map(|m| m.into()) + // TODO: More specific error + .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid COO matrix"), &"a valid COO matrix")) + } +} diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 607cc0cf..cb7cb79b 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -10,6 +10,8 @@ use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKin use nalgebra::Scalar; use num_traits::One; +#[cfg(feature = "serde-serialize")] +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use std::slice::{Iter, IterMut}; /// A CSC representation of a sparse matrix. @@ -524,6 +526,63 @@ impl CscMatrix { } } +#[cfg(feature = "serde-serialize")] +#[derive(Serialize)] +struct CscMatrixSerializationData<'a, T> { + nrows: usize, + ncols: usize, + col_offsets: &'a [usize], + row_indices: &'a [usize], + values: &'a [T], +} + +#[cfg(feature = "serde-serialize")] +impl Serialize for CscMatrix +where + T: Serialize, +{ + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CscMatrixSerializationData { + nrows: self.nrows(), + ncols: self.ncols(), + col_offsets: self.col_offsets(), + row_indices: self.row_indices(), + values: self.values(), + } + .serialize(serializer) + } +} + +#[cfg(feature = "serde-serialize")] +#[derive(Deserialize)] +struct CscMatrixDeserializationData { + nrows: usize, + ncols: usize, + col_offsets: Vec, + row_indices: Vec, + values: Vec, +} + +#[cfg(feature = "serde-serialize")] +impl<'de, T> Deserialize<'de> for CscMatrix +where + T: for<'de2> Deserialize<'de2>, +{ + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let de = CscMatrixDeserializationData::deserialize(deserializer)?; + CscMatrix::try_from_csc_data(de.nrows, de.ncols, de.col_offsets, de.row_indices, de.values) + .map(|m| m.into()) + // TODO: More specific error + .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid CSC matrix"), &"a valid CSC matrix")) + } +} + /// Convert pattern format errors into more meaningful CSC-specific errors. /// /// This ensures that the terminology is consistent: we are talking about rows and columns, diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index bc2a3df0..b36fbb2f 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -9,10 +9,8 @@ use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKin use nalgebra::Scalar; use num_traits::One; - #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; - use std::iter::FromIterator; use std::slice::{Iter, IterMut}; @@ -599,6 +597,16 @@ impl CsrMatrix { } } +#[cfg(feature = "serde-serialize")] +#[derive(Serialize)] +struct CsrMatrixSerializationData<'a, T> { + nrows: usize, + ncols: usize, + row_offsets: &'a [usize], + col_indices: &'a [usize], + values: &'a [T], +} + #[cfg(feature = "serde-serialize")] impl Serialize for CsrMatrix where @@ -608,15 +616,6 @@ where where S: Serializer, { - #[derive(Serialize)] - struct CsrMatrixSerializationData<'a, T> { - nrows: usize, - ncols: usize, - row_offsets: &'a [usize], - col_indices: &'a [usize], - values: &'a [T], - } - CsrMatrixSerializationData { nrows: self.nrows(), ncols: self.ncols(), @@ -628,6 +627,16 @@ where } } +#[cfg(feature = "serde-serialize")] +#[derive(Deserialize)] +struct CsrMatrixDeserializationData { + nrows: usize, + ncols: usize, + row_offsets: Vec, + col_indices: Vec, + values: Vec, +} + #[cfg(feature = "serde-serialize")] impl<'de, T> Deserialize<'de> for CsrMatrix where @@ -637,15 +646,6 @@ where where D: Deserializer<'de>, { - #[derive(Deserialize)] - struct CsrMatrixDeserializationData { - nrows: usize, - ncols: usize, - row_offsets: Vec, - col_indices: Vec, - values: Vec, - } - let de = CsrMatrixDeserializationData::deserialize(deserializer)?; CsrMatrix::try_from_csr_data(de.nrows, de.ncols, de.row_offsets, de.col_indices, de.values) .map(|m| m.into()) diff --git a/nalgebra-sparse/tests/serde.rs b/nalgebra-sparse/tests/serde.rs index ecee76d1..5afe93c5 100644 --- a/nalgebra-sparse/tests/serde.rs +++ b/nalgebra-sparse/tests/serde.rs @@ -6,19 +6,112 @@ compile_error!("Tests must be run with features `proptest-support` and `compare` #[macro_use] pub mod common; +use nalgebra_sparse::coo::CooMatrix; +use nalgebra_sparse::csc::CscMatrix; use nalgebra_sparse::csr::CsrMatrix; use proptest::prelude::*; use serde::{Deserialize, Serialize}; -use crate::common::csr_strategy; +use crate::common::{csc_strategy, csr_strategy}; -fn json_roundtrip Deserialize<'a>>(csr: &CsrMatrix) -> CsrMatrix { +fn json_roundtrip Deserialize<'a>>(csr: &T) -> T { let serialized = serde_json::to_string(csr).unwrap(); - let deserialized: CsrMatrix = serde_json::from_str(&serialized).unwrap(); + let deserialized: T = serde_json::from_str(&serialized).unwrap(); deserialized } +#[test] +fn coo_roundtrip() { + { + // A COO matrix without entries + let matrix = + CooMatrix::::try_from_triplets(3, 2, Vec::new(), Vec::new(), Vec::new()).unwrap(); + + assert_eq!(json_roundtrip(&matrix), matrix); + } + + { + // Arbitrary COO matrix, no duplicates + let i = vec![0, 1, 0, 0, 2]; + let j = vec![0, 2, 1, 3, 3]; + let v = vec![2, 3, 7, 3, 1]; + let matrix = + CooMatrix::::try_from_triplets(3, 5, i.clone(), j.clone(), v.clone()).unwrap(); + + assert_eq!(json_roundtrip(&matrix), matrix); + } +} + +#[test] +fn coo_deserialize_invalid() { + // Valid matrix: {"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,1]} + assert!(serde_json::from_str::>(r#"{"nrows":0,"ncols":0,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":-3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,4,5]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,8,3],"values":[2,3,7,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":5,"row_indices":[0,1,0,0],"col_indices":[0,2,1,8,3],"values":[2,3,7,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":5,"row_indices":[0,10,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,30,3],"values":[2,3,7,3,4]}"#).is_err()); +} + +#[test] +fn coo_deserialize_duplicates() { + assert_eq!( + serde_json::from_str::>( + r#"{"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2,0,1],"col_indices":[0,2,1,3,3,0,2],"values":[2,3,7,3,1,5,6]}"# + ).unwrap(), + CooMatrix::::try_from_triplets( + 3, + 5, + vec![0, 1, 0, 0, 2, 0, 1], + vec![0, 2, 1, 3, 3, 0, 2], + vec![2, 3, 7, 3, 1, 5, 6] + ) + .unwrap() + ); +} + +#[test] +fn csc_roundtrip() { + { + // A CSC matrix with zero explicitly stored entries + let offsets = vec![0, 0, 0, 0]; + let indices = vec![]; + let values = Vec::::new(); + let matrix = CscMatrix::try_from_csc_data(2, 3, offsets, indices, values).unwrap(); + + assert_eq!(json_roundtrip(&matrix), matrix); + } + + { + // An arbitrary CSC matrix + let offsets = vec![0, 2, 2, 5]; + let indices = vec![0, 5, 1, 2, 3]; + let values = vec![0, 1, 2, 3, 4]; + let matrix = + CscMatrix::try_from_csc_data(6, 3, offsets.clone(), indices.clone(), values.clone()) + .unwrap(); + + assert_eq!(json_roundtrip(&matrix), matrix); + } +} + +#[test] +fn csc_deserialize_invalid() { + // Valid matrix: {"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]} + assert!(serde_json::from_str::>(r#"{"nrows":0,"ncols":0,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":-6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4,5]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,8,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3,1,1],"values":[0,1,2,3,4]}"#).is_err()); + // The following actually panics ('range end index 10 out of range for slice of length 5', nalgebra-sparse\src\pattern.rs:156:38) + //assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,10,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); +} + #[test] fn csr_roundtrip() { { @@ -45,16 +138,25 @@ fn csr_roundtrip() { } #[test] -fn invalid_csr_deserialize() { +fn csr_deserialize_invalid() { // Valid matrix: {"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]} + assert!(serde_json::from_str::>(r#"{"nrows":0,"ncols":0,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":-3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4,5]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,8,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3,1,1],"values":[0,1,2,3,4]}"#).is_err()); // The following actually panics ('range end index 10 out of range for slice of length 5', nalgebra-sparse\src\pattern.rs:156:38) //assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,10,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); } proptest! { + #[test] + fn csc_roundtrip_proptest(csc in csc_strategy()) { + prop_assert_eq!(json_roundtrip(&csc), csc); + } + #[test] fn csr_roundtrip_proptest(csr in csr_strategy()) { prop_assert_eq!(json_roundtrip(&csr), csr); From 2a3e657b565dadcd2af422750cd0fb5459be0f6f Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 10:31:50 +0100 Subject: [PATCH 04/20] Rename nrows/ncols args for try_from_*_data functions for consistency --- nalgebra-sparse/src/csc.rs | 14 +++++--------- nalgebra-sparse/src/csr.rs | 14 +++++--------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index cb7cb79b..512be540 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -156,19 +156,15 @@ impl CscMatrix { /// An error is returned if the data given does not conform to the CSC storage format. /// See the documentation for [CscMatrix](struct.CscMatrix.html) for more information. pub fn try_from_csc_data( - num_rows: usize, - num_cols: usize, + nrows: usize, + ncols: usize, col_offsets: Vec, row_indices: Vec, values: Vec, ) -> Result { - let pattern = SparsityPattern::try_from_offsets_and_indices( - num_cols, - num_rows, - col_offsets, - row_indices, - ) - .map_err(pattern_format_error_to_csc_error)?; + let pattern = + SparsityPattern::try_from_offsets_and_indices(ncols, nrows, col_offsets, row_indices) + .map_err(pattern_format_error_to_csc_error)?; Self::try_from_pattern_and_values(pattern, values) } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index b36fbb2f..9e31c90c 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -156,19 +156,15 @@ impl CsrMatrix { /// An error is returned if the data given does not conform to the CSR storage format. /// See the documentation for [CsrMatrix](struct.CsrMatrix.html) for more information. pub fn try_from_csr_data( - num_rows: usize, - num_cols: usize, + nrows: usize, + ncols: usize, row_offsets: Vec, col_indices: Vec, values: Vec, ) -> Result { - let pattern = SparsityPattern::try_from_offsets_and_indices( - num_rows, - num_cols, - row_offsets, - col_indices, - ) - .map_err(pattern_format_error_to_csr_error)?; + let pattern = + SparsityPattern::try_from_offsets_and_indices(nrows, ncols, row_offsets, col_indices) + .map_err(pattern_format_error_to_csr_error)?; Self::try_from_pattern_and_values(pattern, values) } From bfaf29393c546e58c0effee163fb38b0bdc0aca0 Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 10:59:24 +0100 Subject: [PATCH 05/20] Implement Serialize, Deserialize for SparsityPattern --- nalgebra-sparse/src/pattern.rs | 51 ++++++++++++++++++++++++++++++++++ nalgebra-sparse/tests/serde.rs | 42 ++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 85f6bc1a..4243543d 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -4,6 +4,9 @@ use crate::SparseFormatError; use std::error::Error; use std::fmt; +#[cfg(feature = "serde-serialize")] +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + /// A representation of the sparsity pattern of a CSR or CSC matrix. /// /// CSR and CSC matrices store matrices in a very similar fashion. In fact, in a certain sense, @@ -285,6 +288,54 @@ pub enum SparsityPatternFormatError { NonmonotonicMinorIndices, } +#[cfg(feature = "serde-serialize")] +#[derive(Serialize)] +struct SparsityPatternSerializationData<'a> { + major_dim: usize, + minor_dim: usize, + major_offsets: &'a [usize], + minor_indices: &'a [usize], +} + +#[cfg(feature = "serde-serialize")] +impl Serialize for SparsityPattern { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + SparsityPatternSerializationData { + major_dim: self.major_dim(), + minor_dim: self.minor_dim(), + major_offsets: self.major_offsets(), + minor_indices: self.minor_indices(), + } + .serialize(serializer) + } +} + +#[cfg(feature = "serde-serialize")] +#[derive(Deserialize)] +struct SparsityPatternDeserializationData { + major_dim: usize, + minor_dim: usize, + major_offsets: Vec, + minor_indices: Vec, +} + +#[cfg(feature = "serde-serialize")] +impl<'de> Deserialize<'de> for SparsityPattern { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let de = SparsityPatternDeserializationData::deserialize(deserializer)?; + SparsityPattern::try_from_offsets_and_indices(de.major_dim, de.minor_dim, de.major_offsets, de.minor_indices) + .map(|m| m.into()) + // TODO: More specific error + .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid sparsity pattern"), &"a valid sparsity pattern")) + } +} + impl From for SparseFormatError { fn from(err: SparsityPatternFormatError) -> Self { use crate::SparseFormatErrorKind; diff --git a/nalgebra-sparse/tests/serde.rs b/nalgebra-sparse/tests/serde.rs index 5afe93c5..1ce1953f 100644 --- a/nalgebra-sparse/tests/serde.rs +++ b/nalgebra-sparse/tests/serde.rs @@ -9,6 +9,7 @@ pub mod common; use nalgebra_sparse::coo::CooMatrix; use nalgebra_sparse::csc::CscMatrix; use nalgebra_sparse::csr::CsrMatrix; +use nalgebra_sparse::pattern::SparsityPattern; use proptest::prelude::*; use serde::{Deserialize, Serialize}; @@ -21,6 +22,44 @@ fn json_roundtrip Deserialize<'a>>(csr: &T) -> T { deserialized } +#[test] +fn pattern_roundtrip() { + { + // A pattern with zero explicitly stored entries + let pattern = + SparsityPattern::try_from_offsets_and_indices(3, 2, vec![0, 0, 0, 0], Vec::new()) + .unwrap(); + + assert_eq!(json_roundtrip(&pattern), pattern); + } + + { + // Arbitrary pattern + let offsets = vec![0, 2, 2, 5]; + let indices = vec![0, 5, 1, 2, 3]; + let pattern = + SparsityPattern::try_from_offsets_and_indices(3, 6, offsets.clone(), indices.clone()) + .unwrap(); + + assert_eq!(json_roundtrip(&pattern), pattern); + } +} + +#[test] +#[rustfmt::skip] +fn pattern_deserialize_invalid() { + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0,2,2,5],"minor_indices":[0,5,1,2,3]}"#).is_ok()); + assert!(serde_json::from_str::(r#"{"major_dim":0,"minor_dim":0,"major_offsets":[],"minor_indices":[]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 3, 5],"minor_indices":[0, 1, 2, 3, 5]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[1, 2, 2, 5],"minor_indices":[0, 5, 1, 2, 3]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 4],"minor_indices":[0, 5, 1, 2, 3]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2],"minor_indices":[0, 5, 1, 2, 3]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 3, 2, 5],"minor_indices":[0, 1, 2, 3, 4]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 2, 3, 1, 4]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 6, 1, 2, 3]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 5, 2, 2, 3]}"#).is_err()); +} + #[test] fn coo_roundtrip() { { @@ -46,6 +85,7 @@ fn coo_roundtrip() { #[test] fn coo_deserialize_invalid() { // Valid matrix: {"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,1]} + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,1]}"#).is_ok()); assert!(serde_json::from_str::>(r#"{"nrows":0,"ncols":0,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":-3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":5,"row_indices":[0,1,0,0,2],"col_indices":[0,2,1,3,3],"values":[2,3,7,3]}"#).is_err()); @@ -101,6 +141,7 @@ fn csc_roundtrip() { #[test] fn csc_deserialize_invalid() { // Valid matrix: {"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]} + assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_ok()); assert!(serde_json::from_str::>(r#"{"nrows":0,"ncols":0,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":-6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3]}"#).is_err()); @@ -140,6 +181,7 @@ fn csr_roundtrip() { #[test] fn csr_deserialize_invalid() { // Valid matrix: {"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]} + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_ok()); assert!(serde_json::from_str::>(r#"{"nrows":0,"ncols":0,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":-3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3]}"#).is_err()); From e2c33b48ace078fba17a91844a81fc7faaf5ba5f Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 11:09:48 +0100 Subject: [PATCH 06/20] Simplify Deserialize bound --- nalgebra-sparse/src/coo.rs | 2 +- nalgebra-sparse/src/csc.rs | 2 +- nalgebra-sparse/src/csr.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index 35a14083..b84b2cc5 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -320,7 +320,7 @@ struct CooMatrixDeserializationData { #[cfg(feature = "serde-serialize")] impl<'de, T> Deserialize<'de> for CooMatrix where - T: for<'de2> Deserialize<'de2>, + T: Deserialize<'de>, { fn deserialize(deserializer: D) -> Result, D::Error> where diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 512be540..fccc6146 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -565,7 +565,7 @@ struct CscMatrixDeserializationData { #[cfg(feature = "serde-serialize")] impl<'de, T> Deserialize<'de> for CscMatrix where - T: for<'de2> Deserialize<'de2>, + T: Deserialize<'de>, { fn deserialize(deserializer: D) -> Result, D::Error> where diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 9e31c90c..31375340 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -636,7 +636,7 @@ struct CsrMatrixDeserializationData { #[cfg(feature = "serde-serialize")] impl<'de, T> Deserialize<'de> for CsrMatrix where - T: for<'de2> Deserialize<'de2>, + T: Deserialize<'de>, { fn deserialize(deserializer: D) -> Result, D::Error> where From e9b771829246794a33574ed2a44e7f15567152c6 Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 14:07:57 +0100 Subject: [PATCH 07/20] Fix panic in SparsityPattern::try_from_* if major index is out of bounds --- nalgebra-sparse/src/csc.rs | 3 +++ nalgebra-sparse/src/csr.rs | 3 +++ nalgebra-sparse/src/pattern.rs | 11 +++++++++-- nalgebra-sparse/tests/serde.rs | 7 +++---- nalgebra-sparse/tests/unit_tests/pattern.rs | 11 +++++++++++ 5 files changed, 29 insertions(+), 6 deletions(-) diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index fccc6146..e6eb1589 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -606,6 +606,9 @@ fn pattern_format_error_to_csc_error(err: SparsityPatternFormatError) -> SparseF K::InvalidStructure, "Row indices are not monotonically increasing (sorted) within each column.", ), + MajorIndexOutOfBounds => { + E::from_kind_and_msg(K::IndexOutOfBounds, "Column indices are out of bounds.") + } MinorIndexOutOfBounds => { E::from_kind_and_msg(K::IndexOutOfBounds, "Row indices are out of bounds.") } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 31375340..7bd996da 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -677,6 +677,9 @@ fn pattern_format_error_to_csr_error(err: SparsityPatternFormatError) -> SparseF K::InvalidStructure, "Column indices are not monotonically increasing (sorted) within each row.", ), + MajorIndexOutOfBounds => { + E::from_kind_and_msg(K::IndexOutOfBounds, "Row indices are out of bounds.") + } MinorIndexOutOfBounds => { E::from_kind_and_msg(K::IndexOutOfBounds, "Column indices are out of bounds.") } diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 4243543d..1f9dde84 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -156,7 +156,9 @@ impl SparsityPattern { return Err(NonmonotonicOffsets); } - let minor_indices = &minor_indices[range_start..range_end]; + let minor_indices = minor_indices + .get(range_start..range_end) + .ok_or(MajorIndexOutOfBounds)?; // We test for in-bounds, uniqueness and monotonicity at the same time // to ensure that we only visit each minor index once @@ -277,6 +279,8 @@ pub enum SparsityPatternFormatError { InvalidOffsetFirstLast, /// Indicates that the major offsets are not monotonically increasing. NonmonotonicOffsets, + /// One or more major indices are out of bounds. + MajorIndexOutOfBounds, /// One or more minor indices are out of bounds. MinorIndexOutOfBounds, /// One or more duplicate entries were detected. @@ -349,7 +353,7 @@ impl From for SparseFormatError { | NonmonotonicMinorIndices => { SparseFormatError::from_kind_and_error(InvalidStructure, Box::from(err)) } - MinorIndexOutOfBounds => { + MajorIndexOutOfBounds | MinorIndexOutOfBounds => { SparseFormatError::from_kind_and_error(IndexOutOfBounds, Box::from(err)) } PatternDuplicateEntry => SparseFormatError::from_kind_and_error( @@ -373,6 +377,9 @@ impl fmt::Display for SparsityPatternFormatError { SparsityPatternFormatError::NonmonotonicOffsets => { write!(f, "Offsets are not monotonically increasing.") } + SparsityPatternFormatError::MajorIndexOutOfBounds => { + write!(f, "A major index is out of bounds.") + } SparsityPatternFormatError::MinorIndexOutOfBounds => { write!(f, "A minor index is out of bounds.") } diff --git a/nalgebra-sparse/tests/serde.rs b/nalgebra-sparse/tests/serde.rs index 1ce1953f..7842fd1e 100644 --- a/nalgebra-sparse/tests/serde.rs +++ b/nalgebra-sparse/tests/serde.rs @@ -58,6 +58,7 @@ fn pattern_deserialize_invalid() { assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 2, 3, 1, 4]}"#).is_err()); assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 6, 1, 2, 3]}"#).is_err()); assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 5, 2, 2, 3]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 10, 2, 5],"minor_indices":[0, 5, 1, 2, 3]}"#).is_err()); } #[test] @@ -148,8 +149,7 @@ fn csc_deserialize_invalid() { assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4,5]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,8,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3,1,1],"values":[0,1,2,3,4]}"#).is_err()); - // The following actually panics ('range end index 10 out of range for slice of length 5', nalgebra-sparse\src\pattern.rs:156:38) - //assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,10,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,10,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); } @@ -188,8 +188,7 @@ fn csr_deserialize_invalid() { assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4,5]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,8,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3,1,1],"values":[0,1,2,3,4]}"#).is_err()); - // The following actually panics ('range end index 10 out of range for slice of length 5', nalgebra-sparse\src\pattern.rs:156:38) - //assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,10,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,10,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); } diff --git a/nalgebra-sparse/tests/unit_tests/pattern.rs b/nalgebra-sparse/tests/unit_tests/pattern.rs index 310cffae..e2706ee5 100644 --- a/nalgebra-sparse/tests/unit_tests/pattern.rs +++ b/nalgebra-sparse/tests/unit_tests/pattern.rs @@ -133,6 +133,17 @@ fn sparsity_pattern_try_from_invalid_data() { ); } + { + // Major index out of bounds + let offsets = vec![0, 10, 2, 5]; + let indices = vec![0, 1, 2, 3, 4]; + let pattern = SparsityPattern::try_from_offsets_and_indices(3, 6, offsets, indices); + assert_eq!( + pattern, + Err(SparsityPatternFormatError::MajorIndexOutOfBounds) + ); + } + { // Minor index out of bounds let offsets = vec![0, 2, 2, 5]; From 7e67d920a7bddbbd2e6cf8ccb423cfa8f32f85cd Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 17:06:24 +0100 Subject: [PATCH 08/20] Use custom serde errors, make all sparse errs lowercase w/o punctuation --- nalgebra-sparse/src/coo.rs | 21 ++++++++++++-------- nalgebra-sparse/src/csc.rs | 29 ++++++++++++++++------------ nalgebra-sparse/src/csr.rs | 35 +++++++++++++++++++--------------- nalgebra-sparse/src/pattern.rs | 28 +++++++++++++++------------ 4 files changed, 66 insertions(+), 47 deletions(-) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index b84b2cc5..15b23566 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -127,12 +127,12 @@ impl CooMatrix { if row_indices.len() != col_indices.len() { return Err(SparseFormatError::from_kind_and_msg( InvalidStructure, - "Number of row and col indices must be the same.", + "number of row and col indices must be the same", )); } else if col_indices.len() != values.len() { return Err(SparseFormatError::from_kind_and_msg( InvalidStructure, - "Number of col indices and values must be the same.", + "number of col indices and values must be the same", )); } @@ -142,12 +142,12 @@ impl CooMatrix { if !row_indices_in_bounds { Err(SparseFormatError::from_kind_and_msg( IndexOutOfBounds, - "Row index out of bounds.", + "row index out of bounds", )) } else if !col_indices_in_bounds { Err(SparseFormatError::from_kind_and_msg( IndexOutOfBounds, - "Col index out of bounds.", + "col index out of bounds", )) } else { Ok(Self { @@ -327,9 +327,14 @@ where D: Deserializer<'de>, { let de = CooMatrixDeserializationData::deserialize(deserializer)?; - CooMatrix::try_from_triplets(de.nrows, de.ncols, de.row_indices, de.col_indices, de.values) - .map(|m| m.into()) - // TODO: More specific error - .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid COO matrix"), &"a valid COO matrix")) + CooMatrix::try_from_triplets( + de.nrows, + de.ncols, + de.row_indices, + de.col_indices, + de.values, + ) + .map(|m| m.into()) + .map_err(|e| de::Error::custom(e)) } } diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index e6eb1589..54383b01 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -183,7 +183,7 @@ impl CscMatrix { } else { Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "Number of values and row indices must be the same", + "number of values and row indices must be the same", )) } } @@ -572,10 +572,15 @@ where D: Deserializer<'de>, { let de = CscMatrixDeserializationData::deserialize(deserializer)?; - CscMatrix::try_from_csc_data(de.nrows, de.ncols, de.col_offsets, de.row_indices, de.values) - .map(|m| m.into()) - // TODO: More specific error - .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid CSC matrix"), &"a valid CSC matrix")) + CscMatrix::try_from_csc_data( + de.nrows, + de.ncols, + de.col_offsets, + de.row_indices, + de.values, + ) + .map(|m| m.into()) + .map_err(|e| de::Error::custom(e)) } } @@ -592,28 +597,28 @@ fn pattern_format_error_to_csc_error(err: SparsityPatternFormatError) -> SparseF match err { InvalidOffsetArrayLength => E::from_kind_and_msg( K::InvalidStructure, - "Length of col offset array is not equal to ncols + 1.", + "length of col offset array is not equal to ncols + 1", ), InvalidOffsetFirstLast => E::from_kind_and_msg( K::InvalidStructure, - "First or last col offset is inconsistent with format specification.", + "first or last col offset is inconsistent with format specification", ), NonmonotonicOffsets => E::from_kind_and_msg( K::InvalidStructure, - "Col offsets are not monotonically increasing.", + "col offsets are not monotonically increasing", ), NonmonotonicMinorIndices => E::from_kind_and_msg( K::InvalidStructure, - "Row indices are not monotonically increasing (sorted) within each column.", + "row indices are not monotonically increasing (sorted) within each column", ), MajorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "Column indices are out of bounds.") + E::from_kind_and_msg(K::IndexOutOfBounds, "column indices are out of bounds") } MinorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "Row indices are out of bounds.") + E::from_kind_and_msg(K::IndexOutOfBounds, "row indices are out of bounds") } PatternDuplicateEntry => { - E::from_kind_and_msg(K::DuplicateEntry, "Matrix data contains duplicate entries.") + E::from_kind_and_msg(K::DuplicateEntry, "matrix data contains duplicate entries") } } } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 7bd996da..804606ca 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -194,14 +194,14 @@ impl CsrMatrix { if col_indices.len() != values.len() { return Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "Number of values and column indices must be the same", + "number of values and column indices must be the same", )); } if row_offsets.len() == 0 { return Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "Number of offsets should be greater than 0", + "number of offsets should be greater than 0", )); } @@ -210,7 +210,7 @@ impl CsrMatrix { if next_offset > count { return Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "No row offset should be greater than the number of column indices", + "no row offset should be greater than the number of column indices", )); } if offset > next_offset { @@ -254,7 +254,7 @@ impl CsrMatrix { } else { Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "Number of values and column indices must be the same", + "number of values and column indices must be the same", )) } } @@ -643,10 +643,15 @@ where D: Deserializer<'de>, { let de = CsrMatrixDeserializationData::deserialize(deserializer)?; - CsrMatrix::try_from_csr_data(de.nrows, de.ncols, de.row_offsets, de.col_indices, de.values) - .map(|m| m.into()) - // TODO: More specific error - .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid CSR matrix"), &"a valid CSR matrix")) + CsrMatrix::try_from_csr_data( + de.nrows, + de.ncols, + de.row_offsets, + de.col_indices, + de.values, + ) + .map(|m| m.into()) + .map_err(|e| de::Error::custom(e)) } } @@ -663,28 +668,28 @@ fn pattern_format_error_to_csr_error(err: SparsityPatternFormatError) -> SparseF match err { InvalidOffsetArrayLength => E::from_kind_and_msg( K::InvalidStructure, - "Length of row offset array is not equal to nrows + 1.", + "length of row offset array is not equal to nrows + 1", ), InvalidOffsetFirstLast => E::from_kind_and_msg( K::InvalidStructure, - "First or last row offset is inconsistent with format specification.", + "first or last row offset is inconsistent with format specification", ), NonmonotonicOffsets => E::from_kind_and_msg( K::InvalidStructure, - "Row offsets are not monotonically increasing.", + "row offsets are not monotonically increasing", ), NonmonotonicMinorIndices => E::from_kind_and_msg( K::InvalidStructure, - "Column indices are not monotonically increasing (sorted) within each row.", + "column indices are not monotonically increasing (sorted) within each row", ), MajorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "Row indices are out of bounds.") + E::from_kind_and_msg(K::IndexOutOfBounds, "row indices are out of bounds") } MinorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "Column indices are out of bounds.") + E::from_kind_and_msg(K::IndexOutOfBounds, "column indices are out of bounds") } PatternDuplicateEntry => { - E::from_kind_and_msg(K::DuplicateEntry, "Matrix data contains duplicate entries.") + E::from_kind_and_msg(K::DuplicateEntry, "matrix data contains duplicate entries") } } } diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 1f9dde84..1d92dae3 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -259,7 +259,7 @@ impl SparsityPattern { new_offsets, new_indices, ) - .expect("Internal error: Transpose should never fail.") + .expect("internal error: Transpose should never fail") } } @@ -333,10 +333,14 @@ impl<'de> Deserialize<'de> for SparsityPattern { D: Deserializer<'de>, { let de = SparsityPatternDeserializationData::deserialize(deserializer)?; - SparsityPattern::try_from_offsets_and_indices(de.major_dim, de.minor_dim, de.major_offsets, de.minor_indices) - .map(|m| m.into()) - // TODO: More specific error - .map_err(|_e| de::Error::invalid_value(de::Unexpected::Other("invalid sparsity pattern"), &"a valid sparsity pattern")) + SparsityPattern::try_from_offsets_and_indices( + de.major_dim, + de.minor_dim, + de.major_offsets, + de.minor_indices, + ) + .map(|m| m.into()) + .map_err(|e| de::Error::custom(e)) } } @@ -369,27 +373,27 @@ impl fmt::Display for SparsityPatternFormatError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { SparsityPatternFormatError::InvalidOffsetArrayLength => { - write!(f, "Length of offset array is not equal to (major_dim + 1).") + write!(f, "length of offset array is not equal to (major_dim + 1)") } SparsityPatternFormatError::InvalidOffsetFirstLast => { - write!(f, "First or last offset is incompatible with format.") + write!(f, "first or last offset is incompatible with format") } SparsityPatternFormatError::NonmonotonicOffsets => { - write!(f, "Offsets are not monotonically increasing.") + write!(f, "offsets are not monotonically increasing") } SparsityPatternFormatError::MajorIndexOutOfBounds => { - write!(f, "A major index is out of bounds.") + write!(f, "a major index is out of bounds") } SparsityPatternFormatError::MinorIndexOutOfBounds => { - write!(f, "A minor index is out of bounds.") + write!(f, "a minor index is out of bounds") } SparsityPatternFormatError::DuplicateEntry => { - write!(f, "Input data contains duplicate entries.") + write!(f, "input data contains duplicate entries") } SparsityPatternFormatError::NonmonotonicMinorIndices => { write!( f, - "Minor indices are not monotonically increasing within each lane." + "minor indices are not monotonically increasing within each lane" ) } } From 3be81be2e3298c882eed75a49334dfac1dd686f1 Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 17:15:40 +0100 Subject: [PATCH 09/20] Updated more error messages --- nalgebra-sparse/src/convert/serial.rs | 12 ++++++------ nalgebra-sparse/src/factorization/cholesky.rs | 6 +++--- nalgebra-sparse/src/ops/serial/cs.rs | 4 ++-- nalgebra-sparse/src/ops/serial/csc.rs | 6 +++--- nalgebra-sparse/src/ops/serial/mod.rs | 8 ++++---- nalgebra-sparse/src/ops/serial/pattern.rs | 8 ++++---- nalgebra-sparse/src/pattern.rs | 2 +- 7 files changed, 23 insertions(+), 23 deletions(-) diff --git a/nalgebra-sparse/src/convert/serial.rs b/nalgebra-sparse/src/convert/serial.rs index ecbe1dab..9fc3d04e 100644 --- a/nalgebra-sparse/src/convert/serial.rs +++ b/nalgebra-sparse/src/convert/serial.rs @@ -64,7 +64,7 @@ where // TODO: Avoid "try_from" since it validates the data? (requires unsafe, should benchmark // to see if it can be justified for performance reasons) CsrMatrix::try_from_csr_data(coo.nrows(), coo.ncols(), offsets, indices, values) - .expect("Internal error: Invalid CSR data during COO->CSR conversion") + .expect("internal error: invalid CSR data during COO->CSR conversion") } /// Converts a [`CsrMatrix`] to a [`CooMatrix`]. @@ -120,7 +120,7 @@ where // TODO: Consider circumventing the data validity check here // (would require unsafe, should benchmark) CsrMatrix::try_from_csr_data(dense.nrows(), dense.ncols(), row_offsets, col_idx, values) - .expect("Internal error: Invalid CsrMatrix format during dense-> CSR conversion") + .expect("internal error: invalid CsrMatrix format during dense -> CSR conversion") } /// Converts a [`CooMatrix`] to a [`CscMatrix`]. @@ -138,7 +138,7 @@ where // TODO: Avoid "try_from" since it validates the data? (requires unsafe, should benchmark // to see if it can be justified for performance reasons) CscMatrix::try_from_csc_data(coo.nrows(), coo.ncols(), offsets, indices, values) - .expect("Internal error: Invalid CSC data during COO->CSC conversion") + .expect("internal error: invalid CSC data during COO -> CSC conversion") } /// Converts a [`CscMatrix`] to a [`CooMatrix`]. @@ -194,7 +194,7 @@ where // TODO: Consider circumventing the data validity check here // (would require unsafe, should benchmark) CscMatrix::try_from_csc_data(dense.nrows(), dense.ncols(), col_offsets, row_idx, values) - .expect("Internal error: Invalid CscMatrix format during dense-> CSC conversion") + .expect("internal error: invalid CscMatrix format during dense -> CSC conversion") } /// Converts a [`CsrMatrix`] to a [`CscMatrix`]. @@ -212,7 +212,7 @@ where // TODO: Avoid data validity check? CscMatrix::try_from_csc_data(csr.nrows(), csr.ncols(), offsets, indices, values) - .expect("Internal error: Invalid CSC data during CSR->CSC conversion") + .expect("internal error: invalid CSC data during CSR -> CSC conversion") } /// Converts a [`CscMatrix`] to a [`CsrMatrix`]. @@ -230,7 +230,7 @@ where // TODO: Avoid data validity check? CsrMatrix::try_from_csr_data(csc.nrows(), csc.ncols(), offsets, indices, values) - .expect("Internal error: Invalid CSR data during CSC->CSR conversion") + .expect("internal error: invalid CSR data during CSC -> CSR conversion") } fn convert_coo_cs( diff --git a/nalgebra-sparse/src/factorization/cholesky.rs b/nalgebra-sparse/src/factorization/cholesky.rs index 1f653278..b306f104 100644 --- a/nalgebra-sparse/src/factorization/cholesky.rs +++ b/nalgebra-sparse/src/factorization/cholesky.rs @@ -31,7 +31,7 @@ impl CscSymbolicCholesky { assert_eq!( pattern.major_dim(), pattern.minor_dim(), - "Major and minor dimensions must be the same (square matrix)." + "major and minor dimensions must be the same (square matrix)" ); let (l_pattern, u_pattern) = nonzero_pattern(&pattern); Self { @@ -82,7 +82,7 @@ pub enum CholeskyError { impl Display for CholeskyError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "Matrix is not positive definite") + write!(f, "matrix is not positive definite") } } @@ -279,7 +279,7 @@ impl CscCholesky { /// /// Panics if `b` is not square. pub fn solve_mut<'a>(&'a self, b: impl Into>) { - let expect_msg = "If the Cholesky factorization succeeded,\ + let expect_msg = "if the Cholesky factorization succeeded,\ then the triangular solve should never fail"; // Solve LY = B let mut y = b.into(); diff --git a/nalgebra-sparse/src/ops/serial/cs.rs b/nalgebra-sparse/src/ops/serial/cs.rs index 86484053..9cd46152 100644 --- a/nalgebra-sparse/src/ops/serial/cs.rs +++ b/nalgebra-sparse/src/ops/serial/cs.rs @@ -8,7 +8,7 @@ use num_traits::{One, Zero}; fn spmm_cs_unexpected_entry() -> OperationError { OperationError::from_kind_and_message( OperationErrorKind::InvalidPattern, - String::from("Found unexpected entry that is not present in `c`."), + String::from("found unexpected entry that is not present in `c`"), ) } @@ -62,7 +62,7 @@ where fn spadd_cs_unexpected_entry() -> OperationError { OperationError::from_kind_and_message( OperationErrorKind::InvalidPattern, - String::from("Found entry in `op(a)` that is not present in `c`."), + String::from("found entry in `op(a)` that is not present in `c`"), ) } diff --git a/nalgebra-sparse/src/ops/serial/csc.rs b/nalgebra-sparse/src/ops/serial/csc.rs index e5c9ae4e..db7b81ba 100644 --- a/nalgebra-sparse/src/ops/serial/csc.rs +++ b/nalgebra-sparse/src/ops/serial/csc.rs @@ -132,12 +132,12 @@ pub fn spsolve_csc_lower_triangular<'a, T: RealField>( assert_eq!( l_matrix.nrows(), l_matrix.ncols(), - "Matrix must be square for triangular solve." + "matrix must be square for triangular solve" ); assert_eq!( l_matrix.nrows(), b.nrows(), - "Dimension mismatch in sparse lower triangular solver." + "dimension mismatch in sparse lower triangular solver" ); match l { Op::NoOp(a) => spsolve_csc_lower_triangular_no_transpose(a, b), @@ -196,7 +196,7 @@ fn spsolve_csc_lower_triangular_no_transpose( } fn spsolve_encountered_zero_diagonal() -> Result<(), OperationError> { - let message = "Matrix contains at least one diagonal entry that is zero."; + let message = "matrix contains at least one diagonal entry that is zero"; Err(OperationError::from_kind_and_message( OperationErrorKind::Singular, String::from(message), diff --git a/nalgebra-sparse/src/ops/serial/mod.rs b/nalgebra-sparse/src/ops/serial/mod.rs index d8f1a343..3ce47a66 100644 --- a/nalgebra-sparse/src/ops/serial/mod.rs +++ b/nalgebra-sparse/src/ops/serial/mod.rs @@ -108,16 +108,16 @@ impl OperationError { impl fmt::Display for OperationError { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "Sparse matrix operation error: ")?; + write!(f, "sparse matrix operation error: ")?; match self.kind() { OperationErrorKind::InvalidPattern => { - write!(f, "InvalidPattern")?; + write!(f, "invalid pattern")?; } OperationErrorKind::Singular => { - write!(f, "Singular")?; + write!(f, "singular")?; } } - write!(f, " Message: {}", self.message) + write!(f, " message: {}", self.message) } } diff --git a/nalgebra-sparse/src/ops/serial/pattern.rs b/nalgebra-sparse/src/ops/serial/pattern.rs index b73f3375..97137e46 100644 --- a/nalgebra-sparse/src/ops/serial/pattern.rs +++ b/nalgebra-sparse/src/ops/serial/pattern.rs @@ -16,12 +16,12 @@ pub fn spadd_pattern(a: &SparsityPattern, b: &SparsityPattern) -> SparsityPatter assert_eq!( a.major_dim(), b.major_dim(), - "Patterns must have identical major dimensions." + "patterns must have identical major dimensions" ); assert_eq!( a.minor_dim(), b.minor_dim(), - "Patterns must have identical minor dimensions." + "patterns must have identical minor dimensions" ); let mut offsets = Vec::new(); @@ -40,7 +40,7 @@ pub fn spadd_pattern(a: &SparsityPattern, b: &SparsityPattern) -> SparsityPatter // TODO: Consider circumventing format checks? (requires unsafe, should benchmark first) SparsityPattern::try_from_offsets_and_indices(a.major_dim(), a.minor_dim(), offsets, indices) - .expect("Internal error: Pattern must be valid by definition") + .expect("internal error: pattern must be valid by definition") } /// Sparse matrix multiplication pattern construction, `C <- A * B`. @@ -114,7 +114,7 @@ pub fn spmm_csr_pattern(a: &SparsityPattern, b: &SparsityPattern) -> SparsityPat } SparsityPattern::try_from_offsets_and_indices(a.major_dim(), b.minor_dim(), offsets, indices) - .expect("Internal error: Invalid pattern during matrix multiplication pattern construction") + .expect("internal error: invalid pattern during matrix multiplication pattern construction") } /// Iterate over the union of the two sets represented by sorted slices diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 1d92dae3..818cf748 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -259,7 +259,7 @@ impl SparsityPattern { new_offsets, new_indices, ) - .expect("internal error: Transpose should never fail") + .expect("internal error: transpose should never fail") } } From a8fa7f71c0cb918b3dc0cd4d0ff0881fc2248c34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20L=C3=B6schner?= Date: Thu, 9 Dec 2021 18:18:32 +0100 Subject: [PATCH 10/20] Unify separate (de)serialization helper structs by using Cow<'a, [T]> --- nalgebra-sparse/src/coo.rs | 40 +++++++++++++--------------------- nalgebra-sparse/src/csc.rs | 40 +++++++++++++--------------------- nalgebra-sparse/src/csr.rs | 40 +++++++++++++--------------------- nalgebra-sparse/src/pattern.rs | 27 ++++++++--------------- 4 files changed, 54 insertions(+), 93 deletions(-) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index 15b23566..d3612bdc 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -1,6 +1,7 @@ //! An implementation of the COO sparse matrix format. use crate::SparseFormatError; +use std::borrow::Cow; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; @@ -278,19 +279,19 @@ impl CooMatrix { } #[cfg(feature = "serde-serialize")] -#[derive(Serialize)] -struct CooMatrixSerializationData<'a, T> { +#[derive(Serialize, Deserialize)] +struct CooMatrixSerializationData<'a, T: Clone> { nrows: usize, ncols: usize, - row_indices: &'a [usize], - col_indices: &'a [usize], - values: &'a [T], + row_indices: Cow<'a, [usize]>, + col_indices: Cow<'a, [usize]>, + values: Cow<'a, [T]>, } #[cfg(feature = "serde-serialize")] impl Serialize for CooMatrix where - T: Serialize, + T: Serialize + Clone, { fn serialize(&self, serializer: S) -> Result where @@ -299,42 +300,31 @@ where CooMatrixSerializationData { nrows: self.nrows(), ncols: self.ncols(), - row_indices: self.row_indices(), - col_indices: self.col_indices(), - values: self.values(), + row_indices: self.row_indices().into(), + col_indices: self.col_indices().into(), + values: self.values().into(), } .serialize(serializer) } } -#[cfg(feature = "serde-serialize")] -#[derive(Deserialize)] -struct CooMatrixDeserializationData { - nrows: usize, - ncols: usize, - row_indices: Vec, - col_indices: Vec, - values: Vec, -} - #[cfg(feature = "serde-serialize")] impl<'de, T> Deserialize<'de> for CooMatrix where - T: Deserialize<'de>, + T: Deserialize<'de> + Clone, { fn deserialize(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, { - let de = CooMatrixDeserializationData::deserialize(deserializer)?; + let de = CooMatrixSerializationData::deserialize(deserializer)?; CooMatrix::try_from_triplets( de.nrows, de.ncols, - de.row_indices, - de.col_indices, - de.values, + de.row_indices.into(), + de.col_indices.into(), + de.values.into(), ) - .map(|m| m.into()) .map_err(|e| de::Error::custom(e)) } } diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 54383b01..28af852d 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -7,6 +7,7 @@ use crate::cs::{CsLane, CsLaneIter, CsLaneIterMut, CsLaneMut, CsMatrix}; use crate::csr::CsrMatrix; use crate::pattern::{SparsityPattern, SparsityPatternFormatError, SparsityPatternIter}; use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKind}; +use std::borrow::Cow; use nalgebra::Scalar; use num_traits::One; @@ -523,19 +524,19 @@ impl CscMatrix { } #[cfg(feature = "serde-serialize")] -#[derive(Serialize)] -struct CscMatrixSerializationData<'a, T> { +#[derive(Serialize, Deserialize)] +struct CscMatrixSerializationData<'a, T: Clone> { nrows: usize, ncols: usize, - col_offsets: &'a [usize], - row_indices: &'a [usize], - values: &'a [T], + col_offsets: Cow<'a, [usize]>, + row_indices: Cow<'a, [usize]>, + values: Cow<'a, [T]>, } #[cfg(feature = "serde-serialize")] impl Serialize for CscMatrix where - T: Serialize, + T: Serialize + Clone, { fn serialize(&self, serializer: S) -> Result where @@ -544,42 +545,31 @@ where CscMatrixSerializationData { nrows: self.nrows(), ncols: self.ncols(), - col_offsets: self.col_offsets(), - row_indices: self.row_indices(), - values: self.values(), + col_offsets: Cow::Borrowed(self.col_offsets()), + row_indices: Cow::Borrowed(self.row_indices()), + values: Cow::Borrowed(self.values()), } .serialize(serializer) } } -#[cfg(feature = "serde-serialize")] -#[derive(Deserialize)] -struct CscMatrixDeserializationData { - nrows: usize, - ncols: usize, - col_offsets: Vec, - row_indices: Vec, - values: Vec, -} - #[cfg(feature = "serde-serialize")] impl<'de, T> Deserialize<'de> for CscMatrix where - T: Deserialize<'de>, + T: Deserialize<'de> + Clone, { fn deserialize(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, { - let de = CscMatrixDeserializationData::deserialize(deserializer)?; + let de = CscMatrixSerializationData::deserialize(deserializer)?; CscMatrix::try_from_csc_data( de.nrows, de.ncols, - de.col_offsets, - de.row_indices, - de.values, + de.col_offsets.into(), + de.row_indices.into(), + de.values.into(), ) - .map(|m| m.into()) .map_err(|e| de::Error::custom(e)) } } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 804606ca..43fb2118 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -6,6 +6,7 @@ use crate::cs::{CsLane, CsLaneIter, CsLaneIterMut, CsLaneMut, CsMatrix}; use crate::csc::CscMatrix; use crate::pattern::{SparsityPattern, SparsityPatternFormatError, SparsityPatternIter}; use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKind}; +use std::borrow::Cow; use nalgebra::Scalar; use num_traits::One; @@ -594,19 +595,19 @@ impl CsrMatrix { } #[cfg(feature = "serde-serialize")] -#[derive(Serialize)] -struct CsrMatrixSerializationData<'a, T> { +#[derive(Serialize, Deserialize)] +struct CsrMatrixSerializationData<'a, T: Clone> { nrows: usize, ncols: usize, - row_offsets: &'a [usize], - col_indices: &'a [usize], - values: &'a [T], + row_offsets: Cow<'a, [usize]>, + col_indices: Cow<'a, [usize]>, + values: Cow<'a, [T]>, } #[cfg(feature = "serde-serialize")] impl Serialize for CsrMatrix where - T: Serialize, + T: Serialize + Clone, { fn serialize(&self, serializer: S) -> Result where @@ -615,42 +616,31 @@ where CsrMatrixSerializationData { nrows: self.nrows(), ncols: self.ncols(), - row_offsets: self.row_offsets(), - col_indices: self.col_indices(), - values: self.values(), + row_offsets: Cow::Borrowed(self.row_offsets()), + col_indices: Cow::Borrowed(self.col_indices()), + values: Cow::Borrowed(self.values()), } .serialize(serializer) } } -#[cfg(feature = "serde-serialize")] -#[derive(Deserialize)] -struct CsrMatrixDeserializationData { - nrows: usize, - ncols: usize, - row_offsets: Vec, - col_indices: Vec, - values: Vec, -} - #[cfg(feature = "serde-serialize")] impl<'de, T> Deserialize<'de> for CsrMatrix where - T: Deserialize<'de>, + T: Deserialize<'de> + Clone, { fn deserialize(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, { - let de = CsrMatrixDeserializationData::deserialize(deserializer)?; + let de = CsrMatrixSerializationData::deserialize(deserializer)?; CsrMatrix::try_from_csr_data( de.nrows, de.ncols, - de.row_offsets, - de.col_indices, - de.values, + de.row_offsets.into(), + de.col_indices.into(), + de.values.into(), ) - .map(|m| m.into()) .map_err(|e| de::Error::custom(e)) } } diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 818cf748..5f01ea10 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -1,6 +1,7 @@ //! Sparsity patterns for CSR and CSC matrices. use crate::cs::transpose_cs; use crate::SparseFormatError; +use std::borrow::Cow; use std::error::Error; use std::fmt; @@ -293,12 +294,12 @@ pub enum SparsityPatternFormatError { } #[cfg(feature = "serde-serialize")] -#[derive(Serialize)] +#[derive(Serialize, Deserialize)] struct SparsityPatternSerializationData<'a> { major_dim: usize, minor_dim: usize, - major_offsets: &'a [usize], - minor_indices: &'a [usize], + major_offsets: Cow<'a, [usize]>, + minor_indices: Cow<'a, [usize]>, } #[cfg(feature = "serde-serialize")] @@ -310,36 +311,26 @@ impl Serialize for SparsityPattern { SparsityPatternSerializationData { major_dim: self.major_dim(), minor_dim: self.minor_dim(), - major_offsets: self.major_offsets(), - minor_indices: self.minor_indices(), + major_offsets: Cow::Borrowed(self.major_offsets()), + minor_indices: Cow::Borrowed(self.minor_indices()), } .serialize(serializer) } } -#[cfg(feature = "serde-serialize")] -#[derive(Deserialize)] -struct SparsityPatternDeserializationData { - major_dim: usize, - minor_dim: usize, - major_offsets: Vec, - minor_indices: Vec, -} - #[cfg(feature = "serde-serialize")] impl<'de> Deserialize<'de> for SparsityPattern { fn deserialize(deserializer: D) -> Result where D: Deserializer<'de>, { - let de = SparsityPatternDeserializationData::deserialize(deserializer)?; + let de = SparsityPatternSerializationData::deserialize(deserializer)?; SparsityPattern::try_from_offsets_and_indices( de.major_dim, de.minor_dim, - de.major_offsets, - de.minor_indices, + de.major_offsets.into(), + de.minor_indices.into(), ) - .map(|m| m.into()) .map_err(|e| de::Error::custom(e)) } } From 9b87fa4ffa7909b0a72d0e6e1bde6772568def0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20L=C3=B6schner?= Date: Thu, 9 Dec 2021 18:29:45 +0100 Subject: [PATCH 11/20] Add cfg attribute to Cow imports --- nalgebra-sparse/src/coo.rs | 3 ++- nalgebra-sparse/src/csc.rs | 3 ++- nalgebra-sparse/src/csr.rs | 3 ++- nalgebra-sparse/src/pattern.rs | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index d3612bdc..0cebb725 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -1,10 +1,11 @@ //! An implementation of the COO sparse matrix format. use crate::SparseFormatError; -use std::borrow::Cow; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; +#[cfg(feature = "serde-serialize")] +use std::borrow::Cow; /// A COO representation of a sparse matrix. /// diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 28af852d..bd70d4c8 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -7,12 +7,13 @@ use crate::cs::{CsLane, CsLaneIter, CsLaneIterMut, CsLaneMut, CsMatrix}; use crate::csr::CsrMatrix; use crate::pattern::{SparsityPattern, SparsityPatternFormatError, SparsityPatternIter}; use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKind}; -use std::borrow::Cow; use nalgebra::Scalar; use num_traits::One; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; +#[cfg(feature = "serde-serialize")] +use std::borrow::Cow; use std::slice::{Iter, IterMut}; /// A CSC representation of a sparse matrix. diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 43fb2118..bd43927d 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -6,12 +6,13 @@ use crate::cs::{CsLane, CsLaneIter, CsLaneIterMut, CsLaneMut, CsMatrix}; use crate::csc::CscMatrix; use crate::pattern::{SparsityPattern, SparsityPatternFormatError, SparsityPatternIter}; use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKind}; -use std::borrow::Cow; use nalgebra::Scalar; use num_traits::One; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; +#[cfg(feature = "serde-serialize")] +use std::borrow::Cow; use std::iter::FromIterator; use std::slice::{Iter, IterMut}; diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 5f01ea10..a833250c 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -1,12 +1,13 @@ //! Sparsity patterns for CSR and CSC matrices. use crate::cs::transpose_cs; use crate::SparseFormatError; -use std::borrow::Cow; use std::error::Error; use std::fmt; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; +#[cfg(feature = "serde-serialize")] +use std::borrow::Cow; /// A representation of the sparsity pattern of a CSR or CSC matrix. /// From 49eb1bd77846a59c9d13dfffe3e890645850973a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20L=C3=B6schner?= Date: Thu, 9 Dec 2021 21:47:59 +0100 Subject: [PATCH 12/20] CI: Run nalgebra-sparse builds with different feature sets, serde tests --- .github/workflows/nalgebra-ci-build.yml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/nalgebra-ci-build.yml b/.github/workflows/nalgebra-ci-build.yml index 3a56df13..b9df85da 100644 --- a/.github/workflows/nalgebra-ci-build.yml +++ b/.github/workflows/nalgebra-ci-build.yml @@ -38,8 +38,12 @@ jobs: run: cargo build --features serde-serialize - name: Build nalgebra-lapack run: cd nalgebra-lapack; cargo build; - - name: Build nalgebra-sparse + - name: Build nalgebra-sparse --no-default-features + run: cd nalgebra-sparse; cargo build --no-default-features; + - name: Build nalgebra-sparse (default features) run: cd nalgebra-sparse; cargo build; + - name: Build nalgebra-sparse --all-features + run: cd nalgebra-sparse; cargo build --all-features; # Run this on it’s own job because it alone takes a lot of time. # So it’s best to let it run in parallel to the other jobs. build-nalgebra-all-features: @@ -71,10 +75,10 @@ jobs: - name: test nalgebra-sparse # Manifest-path is necessary because cargo otherwise won't correctly forward features # We increase number of proptest cases to hopefully catch more potential bugs - run: PROPTEST_CASES=10000 cargo test --manifest-path=nalgebra-sparse/Cargo.toml --features compare,proptest-support,io + run: PROPTEST_CASES=10000 cargo test --manifest-path=nalgebra-sparse/Cargo.toml --features compare,proptest-support,io,serde-serialize - name: test nalgebra-sparse (slow tests) # Unfortunately, the "slow-tests" take so much time that we need to run them with --release - run: PROPTEST_CASES=10000 cargo test --release --manifest-path=nalgebra-sparse/Cargo.toml --features compare,proptest-support,io,slow-tests slow + run: PROPTEST_CASES=10000 cargo test --release --manifest-path=nalgebra-sparse/Cargo.toml --features compare,proptest-support,io,serde-serialize,slow-tests slow test-nalgebra-macros: runs-on: ubuntu-latest steps: From 837ded932e8e2724cb2c29317a90566793e56256 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20L=C3=B6schner?= Date: Wed, 15 Dec 2021 11:43:35 +0100 Subject: [PATCH 13/20] Replace usage of Cow with generic type --- nalgebra-sparse/src/coo.rs | 26 ++++++++++++-------------- nalgebra-sparse/src/csc.rs | 26 ++++++++++++-------------- nalgebra-sparse/src/csr.rs | 26 ++++++++++++-------------- nalgebra-sparse/src/pattern.rs | 20 +++++++++----------- 4 files changed, 45 insertions(+), 53 deletions(-) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index 0cebb725..3e3c7b4c 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -4,8 +4,6 @@ use crate::SparseFormatError; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -#[cfg(feature = "serde-serialize")] -use std::borrow::Cow; /// A COO representation of a sparse matrix. /// @@ -281,12 +279,12 @@ impl CooMatrix { #[cfg(feature = "serde-serialize")] #[derive(Serialize, Deserialize)] -struct CooMatrixSerializationData<'a, T: Clone> { +struct CooMatrixSerializationData { nrows: usize, ncols: usize, - row_indices: Cow<'a, [usize]>, - col_indices: Cow<'a, [usize]>, - values: Cow<'a, [T]>, + row_indices: Indices, + col_indices: Indices, + values: Values, } #[cfg(feature = "serde-serialize")] @@ -298,12 +296,12 @@ where where S: Serializer, { - CooMatrixSerializationData { + CooMatrixSerializationData::<&[usize], &[T]> { nrows: self.nrows(), ncols: self.ncols(), - row_indices: self.row_indices().into(), - col_indices: self.col_indices().into(), - values: self.values().into(), + row_indices: self.row_indices(), + col_indices: self.col_indices(), + values: self.values(), } .serialize(serializer) } @@ -318,13 +316,13 @@ where where D: Deserializer<'de>, { - let de = CooMatrixSerializationData::deserialize(deserializer)?; + let de = CooMatrixSerializationData::, Vec>::deserialize(deserializer)?; CooMatrix::try_from_triplets( de.nrows, de.ncols, - de.row_indices.into(), - de.col_indices.into(), - de.values.into(), + de.row_indices, + de.col_indices, + de.values, ) .map_err(|e| de::Error::custom(e)) } diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index bd70d4c8..7c7832b5 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -12,8 +12,6 @@ use nalgebra::Scalar; use num_traits::One; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -#[cfg(feature = "serde-serialize")] -use std::borrow::Cow; use std::slice::{Iter, IterMut}; /// A CSC representation of a sparse matrix. @@ -526,12 +524,12 @@ impl CscMatrix { #[cfg(feature = "serde-serialize")] #[derive(Serialize, Deserialize)] -struct CscMatrixSerializationData<'a, T: Clone> { +struct CscMatrixSerializationData { nrows: usize, ncols: usize, - col_offsets: Cow<'a, [usize]>, - row_indices: Cow<'a, [usize]>, - values: Cow<'a, [T]>, + col_offsets: Indices, + row_indices: Indices, + values: Values, } #[cfg(feature = "serde-serialize")] @@ -543,12 +541,12 @@ where where S: Serializer, { - CscMatrixSerializationData { + CscMatrixSerializationData::<&[usize], &[T]> { nrows: self.nrows(), ncols: self.ncols(), - col_offsets: Cow::Borrowed(self.col_offsets()), - row_indices: Cow::Borrowed(self.row_indices()), - values: Cow::Borrowed(self.values()), + col_offsets: self.col_offsets(), + row_indices: self.row_indices(), + values: self.values(), } .serialize(serializer) } @@ -563,13 +561,13 @@ where where D: Deserializer<'de>, { - let de = CscMatrixSerializationData::deserialize(deserializer)?; + let de = CscMatrixSerializationData::, Vec>::deserialize(deserializer)?; CscMatrix::try_from_csc_data( de.nrows, de.ncols, - de.col_offsets.into(), - de.row_indices.into(), - de.values.into(), + de.col_offsets, + de.row_indices, + de.values, ) .map_err(|e| de::Error::custom(e)) } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index bd43927d..786736ff 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -11,8 +11,6 @@ use nalgebra::Scalar; use num_traits::One; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -#[cfg(feature = "serde-serialize")] -use std::borrow::Cow; use std::iter::FromIterator; use std::slice::{Iter, IterMut}; @@ -597,12 +595,12 @@ impl CsrMatrix { #[cfg(feature = "serde-serialize")] #[derive(Serialize, Deserialize)] -struct CsrMatrixSerializationData<'a, T: Clone> { +struct CsrMatrixSerializationData { nrows: usize, ncols: usize, - row_offsets: Cow<'a, [usize]>, - col_indices: Cow<'a, [usize]>, - values: Cow<'a, [T]>, + row_offsets: Indices, + col_indices: Indices, + values: Values, } #[cfg(feature = "serde-serialize")] @@ -614,12 +612,12 @@ where where S: Serializer, { - CsrMatrixSerializationData { + CsrMatrixSerializationData::<&[usize], &[T]> { nrows: self.nrows(), ncols: self.ncols(), - row_offsets: Cow::Borrowed(self.row_offsets()), - col_indices: Cow::Borrowed(self.col_indices()), - values: Cow::Borrowed(self.values()), + row_offsets: self.row_offsets(), + col_indices: self.col_indices(), + values: self.values(), } .serialize(serializer) } @@ -634,13 +632,13 @@ where where D: Deserializer<'de>, { - let de = CsrMatrixSerializationData::deserialize(deserializer)?; + let de = CsrMatrixSerializationData::, Vec>::deserialize(deserializer)?; CsrMatrix::try_from_csr_data( de.nrows, de.ncols, - de.row_offsets.into(), - de.col_indices.into(), - de.values.into(), + de.row_offsets, + de.col_indices, + de.values, ) .map_err(|e| de::Error::custom(e)) } diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index a833250c..f7289438 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -6,8 +6,6 @@ use std::fmt; #[cfg(feature = "serde-serialize")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -#[cfg(feature = "serde-serialize")] -use std::borrow::Cow; /// A representation of the sparsity pattern of a CSR or CSC matrix. /// @@ -296,11 +294,11 @@ pub enum SparsityPatternFormatError { #[cfg(feature = "serde-serialize")] #[derive(Serialize, Deserialize)] -struct SparsityPatternSerializationData<'a> { +struct SparsityPatternSerializationData { major_dim: usize, minor_dim: usize, - major_offsets: Cow<'a, [usize]>, - minor_indices: Cow<'a, [usize]>, + major_offsets: Indices, + minor_indices: Indices, } #[cfg(feature = "serde-serialize")] @@ -309,11 +307,11 @@ impl Serialize for SparsityPattern { where S: Serializer, { - SparsityPatternSerializationData { + SparsityPatternSerializationData::<&[usize]> { major_dim: self.major_dim(), minor_dim: self.minor_dim(), - major_offsets: Cow::Borrowed(self.major_offsets()), - minor_indices: Cow::Borrowed(self.minor_indices()), + major_offsets: self.major_offsets(), + minor_indices: self.minor_indices(), } .serialize(serializer) } @@ -325,12 +323,12 @@ impl<'de> Deserialize<'de> for SparsityPattern { where D: Deserializer<'de>, { - let de = SparsityPatternSerializationData::deserialize(deserializer)?; + let de = SparsityPatternSerializationData::>::deserialize(deserializer)?; SparsityPattern::try_from_offsets_and_indices( de.major_dim, de.minor_dim, - de.major_offsets.into(), - de.minor_indices.into(), + de.major_offsets, + de.minor_indices, ) .map_err(|e| de::Error::custom(e)) } From 647455daddd4fdc3830c17a97282bcd43803a36e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20L=C3=B6schner?= Date: Wed, 15 Dec 2021 11:48:51 +0100 Subject: [PATCH 14/20] Move serialization code to submodules --- nalgebra-sparse/src/coo.rs | 84 ++++++++++++++++---------------- nalgebra-sparse/src/csc.rs | 89 +++++++++++++++++----------------- nalgebra-sparse/src/csr.rs | 89 +++++++++++++++++----------------- nalgebra-sparse/src/pattern.rs | 76 ++++++++++++++--------------- 4 files changed, 170 insertions(+), 168 deletions(-) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index 3e3c7b4c..06788406 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -2,9 +2,6 @@ use crate::SparseFormatError; -#[cfg(feature = "serde-serialize")] -use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; - /// A COO representation of a sparse matrix. /// /// A COO matrix stores entries in coordinate-form, that is triplets `(i, j, v)`, where `i` and `j` @@ -278,52 +275,55 @@ impl CooMatrix { } #[cfg(feature = "serde-serialize")] -#[derive(Serialize, Deserialize)] -struct CooMatrixSerializationData { - nrows: usize, - ncols: usize, - row_indices: Indices, - col_indices: Indices, - values: Values, -} +mod serde_serialize { + use super::CooMatrix; + use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -#[cfg(feature = "serde-serialize")] -impl Serialize for CooMatrix -where - T: Serialize + Clone, -{ - fn serialize(&self, serializer: S) -> Result + #[derive(Serialize, Deserialize)] + struct CooMatrixSerializationData { + nrows: usize, + ncols: usize, + row_indices: Indices, + col_indices: Indices, + values: Values, + } + + impl Serialize for CooMatrix where - S: Serializer, + T: Serialize + Clone, { - CooMatrixSerializationData::<&[usize], &[T]> { - nrows: self.nrows(), - ncols: self.ncols(), - row_indices: self.row_indices(), - col_indices: self.col_indices(), - values: self.values(), + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CooMatrixSerializationData::<&[usize], &[T]> { + nrows: self.nrows(), + ncols: self.ncols(), + row_indices: self.row_indices(), + col_indices: self.col_indices(), + values: self.values(), + } + .serialize(serializer) } - .serialize(serializer) } -} -#[cfg(feature = "serde-serialize")] -impl<'de, T> Deserialize<'de> for CooMatrix -where - T: Deserialize<'de> + Clone, -{ - fn deserialize(deserializer: D) -> Result, D::Error> + impl<'de, T> Deserialize<'de> for CooMatrix where - D: Deserializer<'de>, + T: Deserialize<'de> + Clone, { - let de = CooMatrixSerializationData::, Vec>::deserialize(deserializer)?; - CooMatrix::try_from_triplets( - de.nrows, - de.ncols, - de.row_indices, - de.col_indices, - de.values, - ) - .map_err(|e| de::Error::custom(e)) + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let de = CooMatrixSerializationData::, Vec>::deserialize(deserializer)?; + CooMatrix::try_from_triplets( + de.nrows, + de.ncols, + de.row_indices, + de.col_indices, + de.values, + ) + .map_err(|e| de::Error::custom(e)) + } } } diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 7c7832b5..29861684 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -10,8 +10,6 @@ use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKin use nalgebra::Scalar; use num_traits::One; -#[cfg(feature = "serde-serialize")] -use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use std::slice::{Iter, IterMut}; /// A CSC representation of a sparse matrix. @@ -523,53 +521,56 @@ impl CscMatrix { } #[cfg(feature = "serde-serialize")] -#[derive(Serialize, Deserialize)] -struct CscMatrixSerializationData { - nrows: usize, - ncols: usize, - col_offsets: Indices, - row_indices: Indices, - values: Values, -} +mod serde_serialize { + use super::CscMatrix; + use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -#[cfg(feature = "serde-serialize")] -impl Serialize for CscMatrix -where - T: Serialize + Clone, -{ - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - CscMatrixSerializationData::<&[usize], &[T]> { - nrows: self.nrows(), - ncols: self.ncols(), - col_offsets: self.col_offsets(), - row_indices: self.row_indices(), - values: self.values(), - } - .serialize(serializer) + #[derive(Serialize, Deserialize)] + struct CscMatrixSerializationData { + nrows: usize, + ncols: usize, + col_offsets: Indices, + row_indices: Indices, + values: Values, } -} -#[cfg(feature = "serde-serialize")] -impl<'de, T> Deserialize<'de> for CscMatrix -where - T: Deserialize<'de> + Clone, -{ - fn deserialize(deserializer: D) -> Result, D::Error> + impl Serialize for CscMatrix where - D: Deserializer<'de>, + T: Serialize + Clone, { - let de = CscMatrixSerializationData::, Vec>::deserialize(deserializer)?; - CscMatrix::try_from_csc_data( - de.nrows, - de.ncols, - de.col_offsets, - de.row_indices, - de.values, - ) - .map_err(|e| de::Error::custom(e)) + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CscMatrixSerializationData::<&[usize], &[T]> { + nrows: self.nrows(), + ncols: self.ncols(), + col_offsets: self.col_offsets(), + row_indices: self.row_indices(), + values: self.values(), + } + .serialize(serializer) + } + } + + impl<'de, T> Deserialize<'de> for CscMatrix + where + T: Deserialize<'de> + Clone, + { + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let de = CscMatrixSerializationData::, Vec>::deserialize(deserializer)?; + CscMatrix::try_from_csc_data( + de.nrows, + de.ncols, + de.col_offsets, + de.row_indices, + de.values, + ) + .map_err(|e| de::Error::custom(e)) + } } } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 786736ff..6be91f94 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -9,8 +9,6 @@ use crate::{SparseEntry, SparseEntryMut, SparseFormatError, SparseFormatErrorKin use nalgebra::Scalar; use num_traits::One; -#[cfg(feature = "serde-serialize")] -use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use std::iter::FromIterator; use std::slice::{Iter, IterMut}; @@ -594,53 +592,56 @@ impl CsrMatrix { } #[cfg(feature = "serde-serialize")] -#[derive(Serialize, Deserialize)] -struct CsrMatrixSerializationData { - nrows: usize, - ncols: usize, - row_offsets: Indices, - col_indices: Indices, - values: Values, -} +mod serde_serialize { + use super::CsrMatrix; + use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -#[cfg(feature = "serde-serialize")] -impl Serialize for CsrMatrix -where - T: Serialize + Clone, -{ - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - CsrMatrixSerializationData::<&[usize], &[T]> { - nrows: self.nrows(), - ncols: self.ncols(), - row_offsets: self.row_offsets(), - col_indices: self.col_indices(), - values: self.values(), - } - .serialize(serializer) + #[derive(Serialize, Deserialize)] + struct CsrMatrixSerializationData { + nrows: usize, + ncols: usize, + row_offsets: Indices, + col_indices: Indices, + values: Values, } -} -#[cfg(feature = "serde-serialize")] -impl<'de, T> Deserialize<'de> for CsrMatrix -where - T: Deserialize<'de> + Clone, -{ - fn deserialize(deserializer: D) -> Result, D::Error> + impl Serialize for CsrMatrix where - D: Deserializer<'de>, + T: Serialize + Clone, { - let de = CsrMatrixSerializationData::, Vec>::deserialize(deserializer)?; - CsrMatrix::try_from_csr_data( - de.nrows, - de.ncols, - de.row_offsets, - de.col_indices, - de.values, - ) - .map_err(|e| de::Error::custom(e)) + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CsrMatrixSerializationData::<&[usize], &[T]> { + nrows: self.nrows(), + ncols: self.ncols(), + row_offsets: self.row_offsets(), + col_indices: self.col_indices(), + values: self.values(), + } + .serialize(serializer) + } + } + + impl<'de, T> Deserialize<'de> for CsrMatrix + where + T: Deserialize<'de> + Clone, + { + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let de = CsrMatrixSerializationData::, Vec>::deserialize(deserializer)?; + CsrMatrix::try_from_csr_data( + de.nrows, + de.ncols, + de.row_offsets, + de.col_indices, + de.values, + ) + .map_err(|e| de::Error::custom(e)) + } } } diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index f7289438..1253982b 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -4,9 +4,6 @@ use crate::SparseFormatError; use std::error::Error; use std::fmt; -#[cfg(feature = "serde-serialize")] -use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; - /// A representation of the sparsity pattern of a CSR or CSC matrix. /// /// CSR and CSC matrices store matrices in a very similar fashion. In fact, in a certain sense, @@ -293,44 +290,47 @@ pub enum SparsityPatternFormatError { } #[cfg(feature = "serde-serialize")] -#[derive(Serialize, Deserialize)] -struct SparsityPatternSerializationData { - major_dim: usize, - minor_dim: usize, - major_offsets: Indices, - minor_indices: Indices, -} +mod serde_serialize { + use super::SparsityPattern; + use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -#[cfg(feature = "serde-serialize")] -impl Serialize for SparsityPattern { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - SparsityPatternSerializationData::<&[usize]> { - major_dim: self.major_dim(), - minor_dim: self.minor_dim(), - major_offsets: self.major_offsets(), - minor_indices: self.minor_indices(), - } - .serialize(serializer) + #[derive(Serialize, Deserialize)] + struct SparsityPatternSerializationData { + major_dim: usize, + minor_dim: usize, + major_offsets: Indices, + minor_indices: Indices, } -} -#[cfg(feature = "serde-serialize")] -impl<'de> Deserialize<'de> for SparsityPattern { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let de = SparsityPatternSerializationData::>::deserialize(deserializer)?; - SparsityPattern::try_from_offsets_and_indices( - de.major_dim, - de.minor_dim, - de.major_offsets, - de.minor_indices, - ) - .map_err(|e| de::Error::custom(e)) + impl Serialize for SparsityPattern { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + SparsityPatternSerializationData::<&[usize]> { + major_dim: self.major_dim(), + minor_dim: self.minor_dim(), + major_offsets: self.major_offsets(), + minor_indices: self.minor_indices(), + } + .serialize(serializer) + } + } + + impl<'de> Deserialize<'de> for SparsityPattern { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let de = SparsityPatternSerializationData::>::deserialize(deserializer)?; + SparsityPattern::try_from_offsets_and_indices( + de.major_dim, + de.minor_dim, + de.major_offsets, + de.minor_indices, + ) + .map_err(|e| de::Error::custom(e)) + } } } From 513178e03eb7e54d85aadb01819d051ec23879b7 Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 17:15:40 +0100 Subject: [PATCH 15/20] Revert "Updated more error messages" This reverts commit a42eae45e045c391ede37a7a4328c09cbbb87a0f. --- nalgebra-sparse/src/convert/serial.rs | 12 ++++++------ nalgebra-sparse/src/factorization/cholesky.rs | 6 +++--- nalgebra-sparse/src/ops/serial/cs.rs | 4 ++-- nalgebra-sparse/src/ops/serial/csc.rs | 6 +++--- nalgebra-sparse/src/ops/serial/mod.rs | 8 ++++---- nalgebra-sparse/src/ops/serial/pattern.rs | 8 ++++---- nalgebra-sparse/src/pattern.rs | 2 +- 7 files changed, 23 insertions(+), 23 deletions(-) diff --git a/nalgebra-sparse/src/convert/serial.rs b/nalgebra-sparse/src/convert/serial.rs index 9fc3d04e..ecbe1dab 100644 --- a/nalgebra-sparse/src/convert/serial.rs +++ b/nalgebra-sparse/src/convert/serial.rs @@ -64,7 +64,7 @@ where // TODO: Avoid "try_from" since it validates the data? (requires unsafe, should benchmark // to see if it can be justified for performance reasons) CsrMatrix::try_from_csr_data(coo.nrows(), coo.ncols(), offsets, indices, values) - .expect("internal error: invalid CSR data during COO->CSR conversion") + .expect("Internal error: Invalid CSR data during COO->CSR conversion") } /// Converts a [`CsrMatrix`] to a [`CooMatrix`]. @@ -120,7 +120,7 @@ where // TODO: Consider circumventing the data validity check here // (would require unsafe, should benchmark) CsrMatrix::try_from_csr_data(dense.nrows(), dense.ncols(), row_offsets, col_idx, values) - .expect("internal error: invalid CsrMatrix format during dense -> CSR conversion") + .expect("Internal error: Invalid CsrMatrix format during dense-> CSR conversion") } /// Converts a [`CooMatrix`] to a [`CscMatrix`]. @@ -138,7 +138,7 @@ where // TODO: Avoid "try_from" since it validates the data? (requires unsafe, should benchmark // to see if it can be justified for performance reasons) CscMatrix::try_from_csc_data(coo.nrows(), coo.ncols(), offsets, indices, values) - .expect("internal error: invalid CSC data during COO -> CSC conversion") + .expect("Internal error: Invalid CSC data during COO->CSC conversion") } /// Converts a [`CscMatrix`] to a [`CooMatrix`]. @@ -194,7 +194,7 @@ where // TODO: Consider circumventing the data validity check here // (would require unsafe, should benchmark) CscMatrix::try_from_csc_data(dense.nrows(), dense.ncols(), col_offsets, row_idx, values) - .expect("internal error: invalid CscMatrix format during dense -> CSC conversion") + .expect("Internal error: Invalid CscMatrix format during dense-> CSC conversion") } /// Converts a [`CsrMatrix`] to a [`CscMatrix`]. @@ -212,7 +212,7 @@ where // TODO: Avoid data validity check? CscMatrix::try_from_csc_data(csr.nrows(), csr.ncols(), offsets, indices, values) - .expect("internal error: invalid CSC data during CSR -> CSC conversion") + .expect("Internal error: Invalid CSC data during CSR->CSC conversion") } /// Converts a [`CscMatrix`] to a [`CsrMatrix`]. @@ -230,7 +230,7 @@ where // TODO: Avoid data validity check? CsrMatrix::try_from_csr_data(csc.nrows(), csc.ncols(), offsets, indices, values) - .expect("internal error: invalid CSR data during CSC -> CSR conversion") + .expect("Internal error: Invalid CSR data during CSC->CSR conversion") } fn convert_coo_cs( diff --git a/nalgebra-sparse/src/factorization/cholesky.rs b/nalgebra-sparse/src/factorization/cholesky.rs index b306f104..1f653278 100644 --- a/nalgebra-sparse/src/factorization/cholesky.rs +++ b/nalgebra-sparse/src/factorization/cholesky.rs @@ -31,7 +31,7 @@ impl CscSymbolicCholesky { assert_eq!( pattern.major_dim(), pattern.minor_dim(), - "major and minor dimensions must be the same (square matrix)" + "Major and minor dimensions must be the same (square matrix)." ); let (l_pattern, u_pattern) = nonzero_pattern(&pattern); Self { @@ -82,7 +82,7 @@ pub enum CholeskyError { impl Display for CholeskyError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "matrix is not positive definite") + write!(f, "Matrix is not positive definite") } } @@ -279,7 +279,7 @@ impl CscCholesky { /// /// Panics if `b` is not square. pub fn solve_mut<'a>(&'a self, b: impl Into>) { - let expect_msg = "if the Cholesky factorization succeeded,\ + let expect_msg = "If the Cholesky factorization succeeded,\ then the triangular solve should never fail"; // Solve LY = B let mut y = b.into(); diff --git a/nalgebra-sparse/src/ops/serial/cs.rs b/nalgebra-sparse/src/ops/serial/cs.rs index 9cd46152..86484053 100644 --- a/nalgebra-sparse/src/ops/serial/cs.rs +++ b/nalgebra-sparse/src/ops/serial/cs.rs @@ -8,7 +8,7 @@ use num_traits::{One, Zero}; fn spmm_cs_unexpected_entry() -> OperationError { OperationError::from_kind_and_message( OperationErrorKind::InvalidPattern, - String::from("found unexpected entry that is not present in `c`"), + String::from("Found unexpected entry that is not present in `c`."), ) } @@ -62,7 +62,7 @@ where fn spadd_cs_unexpected_entry() -> OperationError { OperationError::from_kind_and_message( OperationErrorKind::InvalidPattern, - String::from("found entry in `op(a)` that is not present in `c`"), + String::from("Found entry in `op(a)` that is not present in `c`."), ) } diff --git a/nalgebra-sparse/src/ops/serial/csc.rs b/nalgebra-sparse/src/ops/serial/csc.rs index db7b81ba..e5c9ae4e 100644 --- a/nalgebra-sparse/src/ops/serial/csc.rs +++ b/nalgebra-sparse/src/ops/serial/csc.rs @@ -132,12 +132,12 @@ pub fn spsolve_csc_lower_triangular<'a, T: RealField>( assert_eq!( l_matrix.nrows(), l_matrix.ncols(), - "matrix must be square for triangular solve" + "Matrix must be square for triangular solve." ); assert_eq!( l_matrix.nrows(), b.nrows(), - "dimension mismatch in sparse lower triangular solver" + "Dimension mismatch in sparse lower triangular solver." ); match l { Op::NoOp(a) => spsolve_csc_lower_triangular_no_transpose(a, b), @@ -196,7 +196,7 @@ fn spsolve_csc_lower_triangular_no_transpose( } fn spsolve_encountered_zero_diagonal() -> Result<(), OperationError> { - let message = "matrix contains at least one diagonal entry that is zero"; + let message = "Matrix contains at least one diagonal entry that is zero."; Err(OperationError::from_kind_and_message( OperationErrorKind::Singular, String::from(message), diff --git a/nalgebra-sparse/src/ops/serial/mod.rs b/nalgebra-sparse/src/ops/serial/mod.rs index 3ce47a66..d8f1a343 100644 --- a/nalgebra-sparse/src/ops/serial/mod.rs +++ b/nalgebra-sparse/src/ops/serial/mod.rs @@ -108,16 +108,16 @@ impl OperationError { impl fmt::Display for OperationError { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "sparse matrix operation error: ")?; + write!(f, "Sparse matrix operation error: ")?; match self.kind() { OperationErrorKind::InvalidPattern => { - write!(f, "invalid pattern")?; + write!(f, "InvalidPattern")?; } OperationErrorKind::Singular => { - write!(f, "singular")?; + write!(f, "Singular")?; } } - write!(f, " message: {}", self.message) + write!(f, " Message: {}", self.message) } } diff --git a/nalgebra-sparse/src/ops/serial/pattern.rs b/nalgebra-sparse/src/ops/serial/pattern.rs index 97137e46..b73f3375 100644 --- a/nalgebra-sparse/src/ops/serial/pattern.rs +++ b/nalgebra-sparse/src/ops/serial/pattern.rs @@ -16,12 +16,12 @@ pub fn spadd_pattern(a: &SparsityPattern, b: &SparsityPattern) -> SparsityPatter assert_eq!( a.major_dim(), b.major_dim(), - "patterns must have identical major dimensions" + "Patterns must have identical major dimensions." ); assert_eq!( a.minor_dim(), b.minor_dim(), - "patterns must have identical minor dimensions" + "Patterns must have identical minor dimensions." ); let mut offsets = Vec::new(); @@ -40,7 +40,7 @@ pub fn spadd_pattern(a: &SparsityPattern, b: &SparsityPattern) -> SparsityPatter // TODO: Consider circumventing format checks? (requires unsafe, should benchmark first) SparsityPattern::try_from_offsets_and_indices(a.major_dim(), a.minor_dim(), offsets, indices) - .expect("internal error: pattern must be valid by definition") + .expect("Internal error: Pattern must be valid by definition") } /// Sparse matrix multiplication pattern construction, `C <- A * B`. @@ -114,7 +114,7 @@ pub fn spmm_csr_pattern(a: &SparsityPattern, b: &SparsityPattern) -> SparsityPat } SparsityPattern::try_from_offsets_and_indices(a.major_dim(), b.minor_dim(), offsets, indices) - .expect("internal error: invalid pattern during matrix multiplication pattern construction") + .expect("Internal error: Invalid pattern during matrix multiplication pattern construction") } /// Iterate over the union of the two sets represented by sorted slices diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 1253982b..da16df7c 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -256,7 +256,7 @@ impl SparsityPattern { new_offsets, new_indices, ) - .expect("internal error: transpose should never fail") + .expect("internal error: Transpose should never fail") } } From fe70a80e417f1daefba2c9117929d33283c891cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20L=C3=B6schner?= Date: Tue, 28 Dec 2021 12:12:31 +0100 Subject: [PATCH 16/20] Partial revert "Use custom serde errors, make all sparse errs lowercase" --- nalgebra-sparse/src/coo.rs | 8 ++++---- nalgebra-sparse/src/csc.rs | 16 ++++++++-------- nalgebra-sparse/src/csr.rs | 22 +++++++++++----------- nalgebra-sparse/src/pattern.rs | 16 ++++++++-------- 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index 06788406..4ad382dc 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -124,12 +124,12 @@ impl CooMatrix { if row_indices.len() != col_indices.len() { return Err(SparseFormatError::from_kind_and_msg( InvalidStructure, - "number of row and col indices must be the same", + "Number of row and col indices must be the same.", )); } else if col_indices.len() != values.len() { return Err(SparseFormatError::from_kind_and_msg( InvalidStructure, - "number of col indices and values must be the same", + "Number of col indices and values must be the same.", )); } @@ -139,12 +139,12 @@ impl CooMatrix { if !row_indices_in_bounds { Err(SparseFormatError::from_kind_and_msg( IndexOutOfBounds, - "row index out of bounds", + "Row index out of bounds.", )) } else if !col_indices_in_bounds { Err(SparseFormatError::from_kind_and_msg( IndexOutOfBounds, - "col index out of bounds", + "Col index out of bounds.", )) } else { Ok(Self { diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 29861684..c3c843c7 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -181,7 +181,7 @@ impl CscMatrix { } else { Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "number of values and row indices must be the same", + "Number of values and row indices must be the same", )) } } @@ -587,28 +587,28 @@ fn pattern_format_error_to_csc_error(err: SparsityPatternFormatError) -> SparseF match err { InvalidOffsetArrayLength => E::from_kind_and_msg( K::InvalidStructure, - "length of col offset array is not equal to ncols + 1", + "Length of col offset array is not equal to ncols + 1.", ), InvalidOffsetFirstLast => E::from_kind_and_msg( K::InvalidStructure, - "first or last col offset is inconsistent with format specification", + "First or last col offset is inconsistent with format specification.", ), NonmonotonicOffsets => E::from_kind_and_msg( K::InvalidStructure, - "col offsets are not monotonically increasing", + "Col offsets are not monotonically increasing.", ), NonmonotonicMinorIndices => E::from_kind_and_msg( K::InvalidStructure, - "row indices are not monotonically increasing (sorted) within each column", + "Row indices are not monotonically increasing (sorted) within each column.", ), MajorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "column indices are out of bounds") + E::from_kind_and_msg(K::IndexOutOfBounds, "Column indices are out of bounds.") } MinorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "row indices are out of bounds") + E::from_kind_and_msg(K::IndexOutOfBounds, "Row indices are out of bounds.") } PatternDuplicateEntry => { - E::from_kind_and_msg(K::DuplicateEntry, "matrix data contains duplicate entries") + E::from_kind_and_msg(K::DuplicateEntry, "Matrix data contains duplicate entries.") } } } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 6be91f94..88c335b2 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -192,14 +192,14 @@ impl CsrMatrix { if col_indices.len() != values.len() { return Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "number of values and column indices must be the same", + "Number of values and column indices must be the same", )); } if row_offsets.len() == 0 { return Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "number of offsets should be greater than 0", + "Number of offsets should be greater than 0", )); } @@ -208,7 +208,7 @@ impl CsrMatrix { if next_offset > count { return Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "no row offset should be greater than the number of column indices", + "No row offset should be greater than the number of column indices", )); } if offset > next_offset { @@ -252,7 +252,7 @@ impl CsrMatrix { } else { Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, - "number of values and column indices must be the same", + "Number of values and column indices must be the same", )) } } @@ -658,28 +658,28 @@ fn pattern_format_error_to_csr_error(err: SparsityPatternFormatError) -> SparseF match err { InvalidOffsetArrayLength => E::from_kind_and_msg( K::InvalidStructure, - "length of row offset array is not equal to nrows + 1", + "Length of row offset array is not equal to nrows + 1.", ), InvalidOffsetFirstLast => E::from_kind_and_msg( K::InvalidStructure, - "first or last row offset is inconsistent with format specification", + "First or last row offset is inconsistent with format specification.", ), NonmonotonicOffsets => E::from_kind_and_msg( K::InvalidStructure, - "row offsets are not monotonically increasing", + "Row offsets are not monotonically increasing.", ), NonmonotonicMinorIndices => E::from_kind_and_msg( K::InvalidStructure, - "column indices are not monotonically increasing (sorted) within each row", + "Column indices are not monotonically increasing (sorted) within each row.", ), MajorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "row indices are out of bounds") + E::from_kind_and_msg(K::IndexOutOfBounds, "Row indices are out of bounds.") } MinorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "column indices are out of bounds") + E::from_kind_and_msg(K::IndexOutOfBounds, "Column indices are out of bounds.") } PatternDuplicateEntry => { - E::from_kind_and_msg(K::DuplicateEntry, "matrix data contains duplicate entries") + E::from_kind_and_msg(K::DuplicateEntry, "Matrix data contains duplicate entries.") } } } diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index da16df7c..82a93ffd 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -256,7 +256,7 @@ impl SparsityPattern { new_offsets, new_indices, ) - .expect("internal error: Transpose should never fail") + .expect("Internal error: Transpose should never fail.") } } @@ -363,27 +363,27 @@ impl fmt::Display for SparsityPatternFormatError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { SparsityPatternFormatError::InvalidOffsetArrayLength => { - write!(f, "length of offset array is not equal to (major_dim + 1)") + write!(f, "Length of offset array is not equal to (major_dim + 1).") } SparsityPatternFormatError::InvalidOffsetFirstLast => { - write!(f, "first or last offset is incompatible with format") + write!(f, "First or last offset is incompatible with format.") } SparsityPatternFormatError::NonmonotonicOffsets => { - write!(f, "offsets are not monotonically increasing") + write!(f, "Offsets are not monotonically increasing.") } SparsityPatternFormatError::MajorIndexOutOfBounds => { - write!(f, "a major index is out of bounds") + write!(f, "A major index is out of bounds.") } SparsityPatternFormatError::MinorIndexOutOfBounds => { - write!(f, "a minor index is out of bounds") + write!(f, "A minor index is out of bounds.") } SparsityPatternFormatError::DuplicateEntry => { - write!(f, "input data contains duplicate entries") + write!(f, "Input data contains duplicate entries.") } SparsityPatternFormatError::NonmonotonicMinorIndices => { write!( f, - "minor indices are not monotonically increasing within each lane" + "Minor indices are not monotonically increasing within each lane." ) } } From 583fde05fe8233cedb2e83280a66d453b73295f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20L=C3=B6schner?= Date: Tue, 28 Dec 2021 12:36:52 +0100 Subject: [PATCH 17/20] Add comment explaining intermediate types for serialization --- nalgebra-sparse/src/coo.rs | 15 +++++++++++++++ nalgebra-sparse/src/csc.rs | 15 +++++++++++++++ nalgebra-sparse/src/csr.rs | 15 +++++++++++++++ nalgebra-sparse/src/pattern.rs | 15 +++++++++++++++ 4 files changed, 60 insertions(+) diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index 4ad382dc..91ba207d 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -279,6 +279,21 @@ mod serde_serialize { use super::CooMatrix; use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + /// This is an intermediate type for (de)serializing `CooMatrix`. + /// + /// Deserialization requires using a `try_from_*` function for validation. We could have used + /// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows + /// to directly serialize/deserialize the original fields and combine it with validation. + /// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` + /// types. Instead, we decided that we want a more human-readable serialization format using + /// field names like `row_indices` and `col_indices`. The easiest way to achieve this is to + /// introduce an intermediate type. It also allows the serialization format to stay constant + /// even if the internal layout in `nalgebra` changes. + /// + /// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned + /// storage). Therefore, we use generic arguments to allow using slices during serialization and + /// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices + /// and `Vec`s should always (de)serialize identically. #[derive(Serialize, Deserialize)] struct CooMatrixSerializationData { nrows: usize, diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index c3c843c7..7c9bb74a 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -525,6 +525,21 @@ mod serde_serialize { use super::CscMatrix; use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + /// This is an intermediate type for (de)serializing `CscMatrix`. + /// + /// Deserialization requires using a `try_from_*` function for validation. We could have used + /// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows + /// to directly serialize/deserialize the original fields and combine it with validation. + /// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` + /// types. Instead, we decided that we want a more human-readable serialization format using + /// field names like `col_offsets` and `row_indices`. The easiest way to achieve this is to + /// introduce an intermediate type. It also allows the serialization format to stay constant + /// even if the internal layout in `nalgebra` changes. + /// + /// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned + /// storage). Therefore, we use generic arguments to allow using slices during serialization and + /// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices + /// and `Vec`s should always (de)serialize identically. #[derive(Serialize, Deserialize)] struct CscMatrixSerializationData { nrows: usize, diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 88c335b2..eb35b335 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -596,6 +596,21 @@ mod serde_serialize { use super::CsrMatrix; use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + /// This is an intermediate type for (de)serializing `CsrMatrix`. + /// + /// Deserialization requires using a `try_from_*` function for validation. We could have used + /// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows + /// to directly serialize/deserialize the original fields and combine it with validation. + /// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` + /// types. Instead, we decided that we want a more human-readable serialization format using + /// field names like `row_offsets` and `cal_indices`. The easiest way to achieve this is to + /// introduce an intermediate type. It also allows the serialization format to stay constant + /// even if the internal layout in `nalgebra` changes. + /// + /// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned + /// storage). Therefore, we use generic arguments to allow using slices during serialization and + /// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices + /// and `Vec`s should always (de)serialize identically. #[derive(Serialize, Deserialize)] struct CsrMatrixSerializationData { nrows: usize, diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 82a93ffd..2bc9e939 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -294,6 +294,21 @@ mod serde_serialize { use super::SparsityPattern; use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + /// This is an intermediate type for (de)serializing `SparsityPattern`. + /// + /// Deserialization requires using a `try_from_*` function for validation. We could have used + /// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows + /// to directly serialize/deserialize the original fields and combine it with validation. + /// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` + /// types. Instead, we decided that we want a more human-readable serialization format using + /// field names like `major_offsets` and `minor_indices`. The easiest way to achieve this is to + /// introduce an intermediate type. It also allows the serialization format to stay constant + /// even when the internal layout in `nalgebra` changes. + /// + /// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned + /// storage). Therefore, we use generic arguments to allow using slices during serialization and + /// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices + /// and `Vec`s should always (de)serialize identically. #[derive(Serialize, Deserialize)] struct SparsityPatternSerializationData { major_dim: usize, From 38989ed5f0f2b2cd9ab24413d251fcd5bca652bd Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 4 Jan 2022 15:15:52 +0100 Subject: [PATCH 18/20] Move sparse matrix serialization to separate files --- nalgebra-sparse/src/coo.rs | 72 +------------------ nalgebra-sparse/src/coo/coo_serde.rs | 65 +++++++++++++++++ nalgebra-sparse/src/csc.rs | 72 +------------------ nalgebra-sparse/src/csc/csc_serde.rs | 65 +++++++++++++++++ nalgebra-sparse/src/csr.rs | 73 ++------------------ nalgebra-sparse/src/csr/csr_serde.rs | 65 +++++++++++++++++ nalgebra-sparse/src/pattern.rs | 64 ++--------------- nalgebra-sparse/src/pattern/pattern_serde.rs | 56 +++++++++++++++ 8 files changed, 265 insertions(+), 267 deletions(-) create mode 100644 nalgebra-sparse/src/coo/coo_serde.rs create mode 100644 nalgebra-sparse/src/csc/csc_serde.rs create mode 100644 nalgebra-sparse/src/csr/csr_serde.rs create mode 100644 nalgebra-sparse/src/pattern/pattern_serde.rs diff --git a/nalgebra-sparse/src/coo.rs b/nalgebra-sparse/src/coo.rs index 91ba207d..2b302e37 100644 --- a/nalgebra-sparse/src/coo.rs +++ b/nalgebra-sparse/src/coo.rs @@ -1,5 +1,8 @@ //! An implementation of the COO sparse matrix format. +#[cfg(feature = "serde-serialize")] +mod coo_serde; + use crate::SparseFormatError; /// A COO representation of a sparse matrix. @@ -273,72 +276,3 @@ impl CooMatrix { (self.row_indices, self.col_indices, self.values) } } - -#[cfg(feature = "serde-serialize")] -mod serde_serialize { - use super::CooMatrix; - use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; - - /// This is an intermediate type for (de)serializing `CooMatrix`. - /// - /// Deserialization requires using a `try_from_*` function for validation. We could have used - /// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows - /// to directly serialize/deserialize the original fields and combine it with validation. - /// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` - /// types. Instead, we decided that we want a more human-readable serialization format using - /// field names like `row_indices` and `col_indices`. The easiest way to achieve this is to - /// introduce an intermediate type. It also allows the serialization format to stay constant - /// even if the internal layout in `nalgebra` changes. - /// - /// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned - /// storage). Therefore, we use generic arguments to allow using slices during serialization and - /// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices - /// and `Vec`s should always (de)serialize identically. - #[derive(Serialize, Deserialize)] - struct CooMatrixSerializationData { - nrows: usize, - ncols: usize, - row_indices: Indices, - col_indices: Indices, - values: Values, - } - - impl Serialize for CooMatrix - where - T: Serialize + Clone, - { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - CooMatrixSerializationData::<&[usize], &[T]> { - nrows: self.nrows(), - ncols: self.ncols(), - row_indices: self.row_indices(), - col_indices: self.col_indices(), - values: self.values(), - } - .serialize(serializer) - } - } - - impl<'de, T> Deserialize<'de> for CooMatrix - where - T: Deserialize<'de> + Clone, - { - fn deserialize(deserializer: D) -> Result, D::Error> - where - D: Deserializer<'de>, - { - let de = CooMatrixSerializationData::, Vec>::deserialize(deserializer)?; - CooMatrix::try_from_triplets( - de.nrows, - de.ncols, - de.row_indices, - de.col_indices, - de.values, - ) - .map_err(|e| de::Error::custom(e)) - } - } -} diff --git a/nalgebra-sparse/src/coo/coo_serde.rs b/nalgebra-sparse/src/coo/coo_serde.rs new file mode 100644 index 00000000..7ffcdf4a --- /dev/null +++ b/nalgebra-sparse/src/coo/coo_serde.rs @@ -0,0 +1,65 @@ +use crate::coo::CooMatrix; +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + +/// This is an intermediate type for (de)serializing `CooMatrix`. +/// +/// Deserialization requires using a `try_from_*` function for validation. We could have used +/// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows +/// to directly serialize/deserialize the original fields and combine it with validation. +/// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` +/// types. Instead, we decided that we want a more human-readable serialization format using +/// field names like `row_indices` and `col_indices`. The easiest way to achieve this is to +/// introduce an intermediate type. It also allows the serialization format to stay constant +/// even if the internal layout in `nalgebra` changes. +/// +/// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned +/// storage). Therefore, we use generic arguments to allow using slices during serialization and +/// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices +/// and `Vec`s should always (de)serialize identically. +#[derive(Serialize, Deserialize)] +struct CooMatrixSerializationData { + nrows: usize, + ncols: usize, + row_indices: Indices, + col_indices: Indices, + values: Values, +} + +impl Serialize for CooMatrix +where + T: Serialize + Clone, +{ + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CooMatrixSerializationData::<&[usize], &[T]> { + nrows: self.nrows(), + ncols: self.ncols(), + row_indices: self.row_indices(), + col_indices: self.col_indices(), + values: self.values(), + } + .serialize(serializer) + } +} + +impl<'de, T> Deserialize<'de> for CooMatrix +where + T: Deserialize<'de> + Clone, +{ + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let de = CooMatrixSerializationData::, Vec>::deserialize(deserializer)?; + CooMatrix::try_from_triplets( + de.nrows, + de.ncols, + de.row_indices, + de.col_indices, + de.values, + ) + .map_err(|e| de::Error::custom(e)) + } +} diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 7c9bb74a..85e4013c 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -3,6 +3,9 @@ //! This is the module-level documentation. See [`CscMatrix`] for the main documentation of the //! CSC implementation. +#[cfg(feature = "serde-serialize")] +mod csc_serde; + use crate::cs::{CsLane, CsLaneIter, CsLaneIterMut, CsLaneMut, CsMatrix}; use crate::csr::CsrMatrix; use crate::pattern::{SparsityPattern, SparsityPatternFormatError, SparsityPatternIter}; @@ -520,75 +523,6 @@ impl CscMatrix { } } -#[cfg(feature = "serde-serialize")] -mod serde_serialize { - use super::CscMatrix; - use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; - - /// This is an intermediate type for (de)serializing `CscMatrix`. - /// - /// Deserialization requires using a `try_from_*` function for validation. We could have used - /// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows - /// to directly serialize/deserialize the original fields and combine it with validation. - /// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` - /// types. Instead, we decided that we want a more human-readable serialization format using - /// field names like `col_offsets` and `row_indices`. The easiest way to achieve this is to - /// introduce an intermediate type. It also allows the serialization format to stay constant - /// even if the internal layout in `nalgebra` changes. - /// - /// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned - /// storage). Therefore, we use generic arguments to allow using slices during serialization and - /// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices - /// and `Vec`s should always (de)serialize identically. - #[derive(Serialize, Deserialize)] - struct CscMatrixSerializationData { - nrows: usize, - ncols: usize, - col_offsets: Indices, - row_indices: Indices, - values: Values, - } - - impl Serialize for CscMatrix - where - T: Serialize + Clone, - { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - CscMatrixSerializationData::<&[usize], &[T]> { - nrows: self.nrows(), - ncols: self.ncols(), - col_offsets: self.col_offsets(), - row_indices: self.row_indices(), - values: self.values(), - } - .serialize(serializer) - } - } - - impl<'de, T> Deserialize<'de> for CscMatrix - where - T: Deserialize<'de> + Clone, - { - fn deserialize(deserializer: D) -> Result, D::Error> - where - D: Deserializer<'de>, - { - let de = CscMatrixSerializationData::, Vec>::deserialize(deserializer)?; - CscMatrix::try_from_csc_data( - de.nrows, - de.ncols, - de.col_offsets, - de.row_indices, - de.values, - ) - .map_err(|e| de::Error::custom(e)) - } - } -} - /// Convert pattern format errors into more meaningful CSC-specific errors. /// /// This ensures that the terminology is consistent: we are talking about rows and columns, diff --git a/nalgebra-sparse/src/csc/csc_serde.rs b/nalgebra-sparse/src/csc/csc_serde.rs new file mode 100644 index 00000000..aab12d47 --- /dev/null +++ b/nalgebra-sparse/src/csc/csc_serde.rs @@ -0,0 +1,65 @@ +use crate::CscMatrix; +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + +/// This is an intermediate type for (de)serializing `CscMatrix`. +/// +/// Deserialization requires using a `try_from_*` function for validation. We could have used +/// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows +/// to directly serialize/deserialize the original fields and combine it with validation. +/// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` +/// types. Instead, we decided that we want a more human-readable serialization format using +/// field names like `col_offsets` and `row_indices`. The easiest way to achieve this is to +/// introduce an intermediate type. It also allows the serialization format to stay constant +/// even if the internal layout in `nalgebra` changes. +/// +/// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned +/// storage). Therefore, we use generic arguments to allow using slices during serialization and +/// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices +/// and `Vec`s should always (de)serialize identically. +#[derive(Serialize, Deserialize)] +struct CscMatrixSerializationData { + nrows: usize, + ncols: usize, + col_offsets: Indices, + row_indices: Indices, + values: Values, +} + +impl Serialize for CscMatrix +where + T: Serialize + Clone, +{ + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CscMatrixSerializationData::<&[usize], &[T]> { + nrows: self.nrows(), + ncols: self.ncols(), + col_offsets: self.col_offsets(), + row_indices: self.row_indices(), + values: self.values(), + } + .serialize(serializer) + } +} + +impl<'de, T> Deserialize<'de> for CscMatrix +where + T: Deserialize<'de> + Clone, +{ + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let de = CscMatrixSerializationData::, Vec>::deserialize(deserializer)?; + CscMatrix::try_from_csc_data( + de.nrows, + de.ncols, + de.col_offsets, + de.row_indices, + de.values, + ) + .map_err(|e| de::Error::custom(e)) + } +} diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index eb35b335..a87f923d 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -2,6 +2,10 @@ //! //! This is the module-level documentation. See [`CsrMatrix`] for the main documentation of the //! CSC implementation. + +#[cfg(feature = "serde-serialize")] +mod csr_serde; + use crate::cs::{CsLane, CsLaneIter, CsLaneIterMut, CsLaneMut, CsMatrix}; use crate::csc::CscMatrix; use crate::pattern::{SparsityPattern, SparsityPatternFormatError, SparsityPatternIter}; @@ -591,75 +595,6 @@ impl CsrMatrix { } } -#[cfg(feature = "serde-serialize")] -mod serde_serialize { - use super::CsrMatrix; - use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; - - /// This is an intermediate type for (de)serializing `CsrMatrix`. - /// - /// Deserialization requires using a `try_from_*` function for validation. We could have used - /// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows - /// to directly serialize/deserialize the original fields and combine it with validation. - /// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` - /// types. Instead, we decided that we want a more human-readable serialization format using - /// field names like `row_offsets` and `cal_indices`. The easiest way to achieve this is to - /// introduce an intermediate type. It also allows the serialization format to stay constant - /// even if the internal layout in `nalgebra` changes. - /// - /// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned - /// storage). Therefore, we use generic arguments to allow using slices during serialization and - /// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices - /// and `Vec`s should always (de)serialize identically. - #[derive(Serialize, Deserialize)] - struct CsrMatrixSerializationData { - nrows: usize, - ncols: usize, - row_offsets: Indices, - col_indices: Indices, - values: Values, - } - - impl Serialize for CsrMatrix - where - T: Serialize + Clone, - { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - CsrMatrixSerializationData::<&[usize], &[T]> { - nrows: self.nrows(), - ncols: self.ncols(), - row_offsets: self.row_offsets(), - col_indices: self.col_indices(), - values: self.values(), - } - .serialize(serializer) - } - } - - impl<'de, T> Deserialize<'de> for CsrMatrix - where - T: Deserialize<'de> + Clone, - { - fn deserialize(deserializer: D) -> Result, D::Error> - where - D: Deserializer<'de>, - { - let de = CsrMatrixSerializationData::, Vec>::deserialize(deserializer)?; - CsrMatrix::try_from_csr_data( - de.nrows, - de.ncols, - de.row_offsets, - de.col_indices, - de.values, - ) - .map_err(|e| de::Error::custom(e)) - } - } -} - /// Convert pattern format errors into more meaningful CSR-specific errors. /// /// This ensures that the terminology is consistent: we are talking about rows and columns, diff --git a/nalgebra-sparse/src/csr/csr_serde.rs b/nalgebra-sparse/src/csr/csr_serde.rs new file mode 100644 index 00000000..1b33fda0 --- /dev/null +++ b/nalgebra-sparse/src/csr/csr_serde.rs @@ -0,0 +1,65 @@ +use crate::CsrMatrix; +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + +/// This is an intermediate type for (de)serializing `CsrMatrix`. +/// +/// Deserialization requires using a `try_from_*` function for validation. We could have used +/// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows +/// to directly serialize/deserialize the original fields and combine it with validation. +/// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` +/// types. Instead, we decided that we want a more human-readable serialization format using +/// field names like `row_offsets` and `cal_indices`. The easiest way to achieve this is to +/// introduce an intermediate type. It also allows the serialization format to stay constant +/// even if the internal layout in `nalgebra` changes. +/// +/// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned +/// storage). Therefore, we use generic arguments to allow using slices during serialization and +/// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices +/// and `Vec`s should always (de)serialize identically. +#[derive(Serialize, Deserialize)] +struct CsrMatrixSerializationData { + nrows: usize, + ncols: usize, + row_offsets: Indices, + col_indices: Indices, + values: Values, +} + +impl Serialize for CsrMatrix +where + T: Serialize + Clone, +{ + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + CsrMatrixSerializationData::<&[usize], &[T]> { + nrows: self.nrows(), + ncols: self.ncols(), + row_offsets: self.row_offsets(), + col_indices: self.col_indices(), + values: self.values(), + } + .serialize(serializer) + } +} + +impl<'de, T> Deserialize<'de> for CsrMatrix +where + T: Deserialize<'de> + Clone, +{ + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let de = CsrMatrixSerializationData::, Vec>::deserialize(deserializer)?; + CsrMatrix::try_from_csr_data( + de.nrows, + de.ncols, + de.row_offsets, + de.col_indices, + de.values, + ) + .map_err(|e| de::Error::custom(e)) + } +} diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 2bc9e939..59fe8949 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -1,4 +1,8 @@ //! Sparsity patterns for CSR and CSC matrices. + +#[cfg(feature = "serde-serialize")] +mod pattern_serde; + use crate::cs::transpose_cs; use crate::SparseFormatError; use std::error::Error; @@ -289,66 +293,6 @@ pub enum SparsityPatternFormatError { NonmonotonicMinorIndices, } -#[cfg(feature = "serde-serialize")] -mod serde_serialize { - use super::SparsityPattern; - use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; - - /// This is an intermediate type for (de)serializing `SparsityPattern`. - /// - /// Deserialization requires using a `try_from_*` function for validation. We could have used - /// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows - /// to directly serialize/deserialize the original fields and combine it with validation. - /// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` - /// types. Instead, we decided that we want a more human-readable serialization format using - /// field names like `major_offsets` and `minor_indices`. The easiest way to achieve this is to - /// introduce an intermediate type. It also allows the serialization format to stay constant - /// even when the internal layout in `nalgebra` changes. - /// - /// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned - /// storage). Therefore, we use generic arguments to allow using slices during serialization and - /// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices - /// and `Vec`s should always (de)serialize identically. - #[derive(Serialize, Deserialize)] - struct SparsityPatternSerializationData { - major_dim: usize, - minor_dim: usize, - major_offsets: Indices, - minor_indices: Indices, - } - - impl Serialize for SparsityPattern { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - SparsityPatternSerializationData::<&[usize]> { - major_dim: self.major_dim(), - minor_dim: self.minor_dim(), - major_offsets: self.major_offsets(), - minor_indices: self.minor_indices(), - } - .serialize(serializer) - } - } - - impl<'de> Deserialize<'de> for SparsityPattern { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let de = SparsityPatternSerializationData::>::deserialize(deserializer)?; - SparsityPattern::try_from_offsets_and_indices( - de.major_dim, - de.minor_dim, - de.major_offsets, - de.minor_indices, - ) - .map_err(|e| de::Error::custom(e)) - } - } -} - impl From for SparseFormatError { fn from(err: SparsityPatternFormatError) -> Self { use crate::SparseFormatErrorKind; diff --git a/nalgebra-sparse/src/pattern/pattern_serde.rs b/nalgebra-sparse/src/pattern/pattern_serde.rs new file mode 100644 index 00000000..e11a550a --- /dev/null +++ b/nalgebra-sparse/src/pattern/pattern_serde.rs @@ -0,0 +1,56 @@ +use crate::pattern::SparsityPattern; +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + +/// This is an intermediate type for (de)serializing `SparsityPattern`. +/// +/// Deserialization requires using a `try_from_*` function for validation. We could have used +/// the `remote = "Self"` trick (https://github.com/serde-rs/serde/issues/1220) which allows +/// to directly serialize/deserialize the original fields and combine it with validation. +/// However, this would lead to nested serialization of the `CsMatrix` and `SparsityPattern` +/// types. Instead, we decided that we want a more human-readable serialization format using +/// field names like `major_offsets` and `minor_indices`. The easiest way to achieve this is to +/// introduce an intermediate type. It also allows the serialization format to stay constant +/// even when the internal layout in `nalgebra` changes. +/// +/// We want to avoid unnecessary copies when serializing (i.e. cloning slices into owned +/// storage). Therefore, we use generic arguments to allow using slices during serialization and +/// owned storage (i.e. `Vec`) during deserialization. Without a major update of serde, slices +/// and `Vec`s should always (de)serialize identically. +#[derive(Serialize, Deserialize)] +struct SparsityPatternSerializationData { + major_dim: usize, + minor_dim: usize, + major_offsets: Indices, + minor_indices: Indices, +} + +impl Serialize for SparsityPattern { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + SparsityPatternSerializationData::<&[usize]> { + major_dim: self.major_dim(), + minor_dim: self.minor_dim(), + major_offsets: self.major_offsets(), + minor_indices: self.minor_indices(), + } + .serialize(serializer) + } +} + +impl<'de> Deserialize<'de> for SparsityPattern { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let de = SparsityPatternSerializationData::>::deserialize(deserializer)?; + SparsityPattern::try_from_offsets_and_indices( + de.major_dim, + de.minor_dim, + de.major_offsets, + de.minor_indices, + ) + .map_err(|e| de::Error::custom(e)) + } +} From 89f1e855bb5a22b401695c1cbe8c350bfd5774dd Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 14:07:57 +0100 Subject: [PATCH 19/20] Revert "Fix panic in SparsityPattern::try_from_* if major index is out of bounds" This reverts commit 12afe2917af4c30fc4a17316e453d0830072642c to avoid conflict with #1015. --- nalgebra-sparse/src/csc.rs | 3 --- nalgebra-sparse/src/csr.rs | 3 --- nalgebra-sparse/src/pattern.rs | 11 ++--------- nalgebra-sparse/tests/serde.rs | 7 ++++--- nalgebra-sparse/tests/unit_tests/pattern.rs | 11 ----------- 5 files changed, 6 insertions(+), 29 deletions(-) diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 85e4013c..9180ae12 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -550,9 +550,6 @@ fn pattern_format_error_to_csc_error(err: SparsityPatternFormatError) -> SparseF K::InvalidStructure, "Row indices are not monotonically increasing (sorted) within each column.", ), - MajorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "Column indices are out of bounds.") - } MinorIndexOutOfBounds => { E::from_kind_and_msg(K::IndexOutOfBounds, "Row indices are out of bounds.") } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index a87f923d..1f52a867 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -622,9 +622,6 @@ fn pattern_format_error_to_csr_error(err: SparsityPatternFormatError) -> SparseF K::InvalidStructure, "Column indices are not monotonically increasing (sorted) within each row.", ), - MajorIndexOutOfBounds => { - E::from_kind_and_msg(K::IndexOutOfBounds, "Row indices are out of bounds.") - } MinorIndexOutOfBounds => { E::from_kind_and_msg(K::IndexOutOfBounds, "Column indices are out of bounds.") } diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 59fe8949..50fae072 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -157,9 +157,7 @@ impl SparsityPattern { return Err(NonmonotonicOffsets); } - let minor_indices = minor_indices - .get(range_start..range_end) - .ok_or(MajorIndexOutOfBounds)?; + let minor_indices = &minor_indices[range_start..range_end]; // We test for in-bounds, uniqueness and monotonicity at the same time // to ensure that we only visit each minor index once @@ -280,8 +278,6 @@ pub enum SparsityPatternFormatError { InvalidOffsetFirstLast, /// Indicates that the major offsets are not monotonically increasing. NonmonotonicOffsets, - /// One or more major indices are out of bounds. - MajorIndexOutOfBounds, /// One or more minor indices are out of bounds. MinorIndexOutOfBounds, /// One or more duplicate entries were detected. @@ -306,7 +302,7 @@ impl From for SparseFormatError { | NonmonotonicMinorIndices => { SparseFormatError::from_kind_and_error(InvalidStructure, Box::from(err)) } - MajorIndexOutOfBounds | MinorIndexOutOfBounds => { + MinorIndexOutOfBounds => { SparseFormatError::from_kind_and_error(IndexOutOfBounds, Box::from(err)) } PatternDuplicateEntry => SparseFormatError::from_kind_and_error( @@ -330,9 +326,6 @@ impl fmt::Display for SparsityPatternFormatError { SparsityPatternFormatError::NonmonotonicOffsets => { write!(f, "Offsets are not monotonically increasing.") } - SparsityPatternFormatError::MajorIndexOutOfBounds => { - write!(f, "A major index is out of bounds.") - } SparsityPatternFormatError::MinorIndexOutOfBounds => { write!(f, "A minor index is out of bounds.") } diff --git a/nalgebra-sparse/tests/serde.rs b/nalgebra-sparse/tests/serde.rs index 7842fd1e..1ce1953f 100644 --- a/nalgebra-sparse/tests/serde.rs +++ b/nalgebra-sparse/tests/serde.rs @@ -58,7 +58,6 @@ fn pattern_deserialize_invalid() { assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 2, 3, 1, 4]}"#).is_err()); assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 6, 1, 2, 3]}"#).is_err()); assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 2, 2, 5],"minor_indices":[0, 5, 2, 2, 3]}"#).is_err()); - assert!(serde_json::from_str::(r#"{"major_dim":3,"minor_dim":6,"major_offsets":[0, 10, 2, 5],"minor_indices":[0, 5, 1, 2, 3]}"#).is_err()); } #[test] @@ -149,7 +148,8 @@ fn csc_deserialize_invalid() { assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4,5]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,8,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3,1,1],"values":[0,1,2,3,4]}"#).is_err()); - assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,10,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + // The following actually panics ('range end index 10 out of range for slice of length 5', nalgebra-sparse\src\pattern.rs:156:38) + //assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,10,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); } @@ -188,7 +188,8 @@ fn csr_deserialize_invalid() { assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4,5]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,8,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,2,2,5],"col_indices":[0,5,1,2,3,1,1],"values":[0,1,2,3,4]}"#).is_err()); - assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,10,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); + // The following actually panics ('range end index 10 out of range for slice of length 5', nalgebra-sparse\src\pattern.rs:156:38) + //assert!(serde_json::from_str::>(r#"{"nrows":3,"ncols":6,"row_offsets":[0,10,2,5],"col_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); assert!(serde_json::from_str::>(r#"{"nrows":6,"ncols":3,"col_offsets":[0,2,2,5],"row_indices":[0,5,1,2,3],"values":[0,1,2,3,4]}"#).is_err()); } diff --git a/nalgebra-sparse/tests/unit_tests/pattern.rs b/nalgebra-sparse/tests/unit_tests/pattern.rs index e2706ee5..310cffae 100644 --- a/nalgebra-sparse/tests/unit_tests/pattern.rs +++ b/nalgebra-sparse/tests/unit_tests/pattern.rs @@ -133,17 +133,6 @@ fn sparsity_pattern_try_from_invalid_data() { ); } - { - // Major index out of bounds - let offsets = vec![0, 10, 2, 5]; - let indices = vec![0, 1, 2, 3, 4]; - let pattern = SparsityPattern::try_from_offsets_and_indices(3, 6, offsets, indices); - assert_eq!( - pattern, - Err(SparsityPatternFormatError::MajorIndexOutOfBounds) - ); - } - { // Minor index out of bounds let offsets = vec![0, 2, 2, 5]; From 99eb8c1589e5d6fcbb48a9746e177511eb55c9d4 Mon Sep 17 00:00:00 2001 From: Fabian Loeschner Date: Tue, 9 Nov 2021 10:31:50 +0100 Subject: [PATCH 20/20] Revert "Rename nrows/ncols args for try_from_*_data functions for consistency" This reverts commit 2a3e657b565dadcd2af422750cd0fb5459be0f6f. --- nalgebra-sparse/src/csc.rs | 14 +++++++++----- nalgebra-sparse/src/csr.rs | 14 +++++++++----- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/nalgebra-sparse/src/csc.rs b/nalgebra-sparse/src/csc.rs index 9180ae12..a461658e 100644 --- a/nalgebra-sparse/src/csc.rs +++ b/nalgebra-sparse/src/csc.rs @@ -157,15 +157,19 @@ impl CscMatrix { /// An error is returned if the data given does not conform to the CSC storage format. /// See the documentation for [CscMatrix](struct.CscMatrix.html) for more information. pub fn try_from_csc_data( - nrows: usize, - ncols: usize, + num_rows: usize, + num_cols: usize, col_offsets: Vec, row_indices: Vec, values: Vec, ) -> Result { - let pattern = - SparsityPattern::try_from_offsets_and_indices(ncols, nrows, col_offsets, row_indices) - .map_err(pattern_format_error_to_csc_error)?; + let pattern = SparsityPattern::try_from_offsets_and_indices( + num_cols, + num_rows, + col_offsets, + row_indices, + ) + .map_err(pattern_format_error_to_csc_error)?; Self::try_from_pattern_and_values(pattern, values) } diff --git a/nalgebra-sparse/src/csr.rs b/nalgebra-sparse/src/csr.rs index 1f52a867..bd35bf6b 100644 --- a/nalgebra-sparse/src/csr.rs +++ b/nalgebra-sparse/src/csr.rs @@ -158,15 +158,19 @@ impl CsrMatrix { /// An error is returned if the data given does not conform to the CSR storage format. /// See the documentation for [CsrMatrix](struct.CsrMatrix.html) for more information. pub fn try_from_csr_data( - nrows: usize, - ncols: usize, + num_rows: usize, + num_cols: usize, row_offsets: Vec, col_indices: Vec, values: Vec, ) -> Result { - let pattern = - SparsityPattern::try_from_offsets_and_indices(nrows, ncols, row_offsets, col_indices) - .map_err(pattern_format_error_to_csr_error)?; + let pattern = SparsityPattern::try_from_offsets_and_indices( + num_rows, + num_cols, + row_offsets, + col_indices, + ) + .map_err(pattern_format_error_to_csr_error)?; Self::try_from_pattern_and_values(pattern, values) }