From a3fb3b40954bad28eec179bee178f9572dba77e1 Mon Sep 17 00:00:00 2001 From: woojucy Date: Sat, 4 Jan 2025 20:53:45 +0900 Subject: [PATCH 01/11] Update CHANGELOG: Fix handling of duplicate terms in SparsePolynomial --- CHANGELOG.md | 2 ++ poly/src/polynomial/univariate/sparse.rs | 31 +++++++++++++++--------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c3034ecd..3cb495c96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ ### Bugfixes +- (`ark-poly`) Fix handling of duplicate terms in `SparsePolynomial::from_coefficients_vec`. Terms with the same degree are now correctly merged, and zero coefficient terms are removed. + ## v0.5.0 - [\#772](https://github.com/arkworks-rs/algebra/pull/772) (`ark-ff`) Implementation of `mul` method for `BigInteger`. diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index 7357ad429..db56c8126 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -236,20 +236,27 @@ impl SparsePolynomial { } /// Constructs a new polynomial from a list of coefficients. - /// The function does not combine like terms and so multiple monomials - /// of the same degree are ignored. pub fn from_coefficients_vec(mut coeffs: Vec<(usize, F)>) -> Self { - // While there are zeros at the end of the coefficient vector, pop them off. - while coeffs.last().map_or(false, |(_, c)| c.is_zero()) { - coeffs.pop(); - } - // Ensure that coeffs are in ascending order. + // Sort terms by degree coeffs.sort_by(|(c1, _), (c2, _)| c1.cmp(c2)); - // Check that either the coefficients vec is empty or that the last coeff is - // non-zero. - assert!(coeffs.last().map_or(true, |(_, c)| !c.is_zero())); - - Self { coeffs } + + // Merge duplicate terms + let mut merged_coeffs: Vec<(usize, F)> = Vec::new(); + for (degree, coeff) in coeffs { + if let Some((last_degree, last_coeff)) = merged_coeffs.last_mut() { + if *last_degree == degree { + *last_coeff += coeff; // Combine coefficients for duplicate degrees + continue; + } + } + merged_coeffs.push((degree, coeff)); + } + + // Remove zero coefficient terms + merged_coeffs.retain(|(_, coeff)| !coeff.is_zero()); + + // Create the SparsePolynomial + Self { coeffs: merged_coeffs } } /// Perform a naive n^2 multiplication of `self` by `other`. From f1d4d2222715a98bbbef2bdb2255f5caeffc910b Mon Sep 17 00:00:00 2001 From: woojucy Date: Sat, 1 Mar 2025 20:33:52 +0900 Subject: [PATCH 02/11] Add pre-validation to SparsePolynomial construction --- CHANGELOG.md | 2 +- poly/src/polynomial/univariate/sparse.rs | 49 +++++++++++++++--------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cb495c96..006456283 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ ### Bugfixes -- (`ark-poly`) Fix handling of duplicate terms in `SparsePolynomial::from_coefficients_vec`. Terms with the same degree are now correctly merged, and zero coefficient terms are removed. +- [\#915](https://github.com/arkworks-rs/algebra/pull/915) (`ark-poly`) Added `is_valid_coefficients_vec` to `SparsePolynomial::from_coefficients_vec` to remove duplicates and zero coefficients before polynomial creation. ## v0.5.0 diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index db56c8126..99b73885a 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -230,33 +230,44 @@ impl Zero for SparsePolynomial { } impl SparsePolynomial { + + // Function to validate that the input coefficient vector is simplified + pub fn is_valid_coefficients_vec(coeffs: &[(usize, F)]) -> bool { + let mut last_degree = None; + for &(degree, ref coeff) in coeffs { + if coeff.is_zero() { + return false; // Zero coefficients are not allowed + } + if let Some(last) = last_degree { + if last == degree { + return false; // Duplicate degrees are not allowed + } + } + last_degree = Some(degree); + } + true + } + /// Constructs a new polynomial from a list of coefficients. pub fn from_coefficients_slice(coeffs: &[(usize, F)]) -> Self { Self::from_coefficients_vec(coeffs.to_vec()) } /// Constructs a new polynomial from a list of coefficients. + /// The function does not combine like terms and so multiple monomials + /// of the same degree are ignored. pub fn from_coefficients_vec(mut coeffs: Vec<(usize, F)>) -> Self { - // Sort terms by degree - coeffs.sort_by(|(c1, _), (c2, _)| c1.cmp(c2)); - - // Merge duplicate terms - let mut merged_coeffs: Vec<(usize, F)> = Vec::new(); - for (degree, coeff) in coeffs { - if let Some((last_degree, last_coeff)) = merged_coeffs.last_mut() { - if *last_degree == degree { - *last_coeff += coeff; // Combine coefficients for duplicate degrees - continue; - } - } - merged_coeffs.push((degree, coeff)); + // While there are zeros at the end of the coefficient vector, pop them off. + while coeffs.last().map_or(false, |(_, c)| c.is_zero()) { + coeffs.pop(); } - - // Remove zero coefficient terms - merged_coeffs.retain(|(_, coeff)| !coeff.is_zero()); - - // Create the SparsePolynomial - Self { coeffs: merged_coeffs } + // Ensure that coeffs are in ascending order. + coeffs.sort_by(|(c1, _), (c2, _)| c1.cmp(c2)); + // Check that either the coefficients vec is empty or that the last coeff is + // non-zero. + assert!(coeffs.last().map_or(true, |(_, c)| !c.is_zero())); + + Self { coeffs } } /// Perform a naive n^2 multiplication of `self` by `other`. From 8148bb27a2e4c9f0cb3277f64dc42ead06b1d2e5 Mon Sep 17 00:00:00 2001 From: Chanyang <124057018+woojucy@users.noreply.github.com> Date: Sun, 2 Mar 2025 03:00:06 +0900 Subject: [PATCH 03/11] Update poly/src/polynomial/univariate/sparse.rs Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com> --- poly/src/polynomial/univariate/sparse.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index 99b73885a..eb90e6e3e 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -232,20 +232,22 @@ impl Zero for SparsePolynomial { impl SparsePolynomial { // Function to validate that the input coefficient vector is simplified - pub fn is_valid_coefficients_vec(coeffs: &[(usize, F)]) -> bool { + pub fn is_valid_coefficients_vec(coeffs: &[(usize, F)]) -> Result<(), CoeffVecError> { let mut last_degree = None; for &(degree, ref coeff) in coeffs { if coeff.is_zero() { - return false; // Zero coefficients are not allowed + // zero term has no effect + return Err(CoeffVecError::ZeroCoefficient); } if let Some(last) = last_degree { if last == degree { - return false; // Duplicate degrees are not allowed + // like terms should be combined + return Err(CoeffVecError::DuplicateDegree); } } last_degree = Some(degree); } - true + Ok(()) } /// Constructs a new polynomial from a list of coefficients. From 887dd95fde0ececb268c22069bf243ab6fa41b77 Mon Sep 17 00:00:00 2001 From: Chanyang <124057018+woojucy@users.noreply.github.com> Date: Sun, 2 Mar 2025 03:00:31 +0900 Subject: [PATCH 04/11] Update poly/src/polynomial/univariate/sparse.rs Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com> --- poly/src/polynomial/univariate/sparse.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index eb90e6e3e..858bf12f2 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -229,6 +229,12 @@ impl Zero for SparsePolynomial { } } +#[derive(Debug)] +pub enum CoeffVecError { + ZeroCoefficient, + DuplicateDegree, +} + impl SparsePolynomial { // Function to validate that the input coefficient vector is simplified From 09f73e808af635527f25058666288e566d7c055b Mon Sep 17 00:00:00 2001 From: Chanyang <124057018+woojucy@users.noreply.github.com> Date: Sun, 2 Mar 2025 03:00:53 +0900 Subject: [PATCH 05/11] Update CHANGELOG.md Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 006456283..dcd9bf22e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ ### Bugfixes -- [\#915](https://github.com/arkworks-rs/algebra/pull/915) (`ark-poly`) Added `is_valid_coefficients_vec` to `SparsePolynomial::from_coefficients_vec` to remove duplicates and zero coefficients before polynomial creation. +- [\#915](https://github.com/arkworks-rs/algebra/pull/915) (`ark-poly`) Added `is_valid_coefficients_vec` to `SparsePolynomial` to remove duplicates and zero coefficients before polynomial creation. ## v0.5.0 From 93f8bfa1852b2759d1352f6ded1957df81d56480 Mon Sep 17 00:00:00 2001 From: Chanyang <124057018+woojucy@users.noreply.github.com> Date: Sun, 2 Mar 2025 03:01:55 +0900 Subject: [PATCH 06/11] Update poly/src/polynomial/univariate/sparse.rs Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com> --- poly/src/polynomial/univariate/sparse.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index 858bf12f2..7ce2a1e1d 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -235,6 +235,12 @@ pub enum CoeffVecError { DuplicateDegree, } +#[derive(Debug)] +pub enum CoeffVecError { + ZeroCoefficient, + DuplicateDegree, +} + impl SparsePolynomial { // Function to validate that the input coefficient vector is simplified From a312f6fd840082d345149ec50a011f35db2ba347 Mon Sep 17 00:00:00 2001 From: Chanyang <124057018+woojucy@users.noreply.github.com> Date: Sun, 2 Mar 2025 03:02:11 +0900 Subject: [PATCH 07/11] Update poly/src/polynomial/univariate/sparse.rs Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com> --- poly/src/polynomial/univariate/sparse.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index 7ce2a1e1d..c3329677f 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -241,6 +241,12 @@ pub enum CoeffVecError { DuplicateDegree, } +#[derive(Debug)] +pub enum CoeffVecError { + ZeroCoefficient, + DuplicateDegree, +} + impl SparsePolynomial { // Function to validate that the input coefficient vector is simplified From f8781e771bf20197d2c86a2b45c7f75c01496c34 Mon Sep 17 00:00:00 2001 From: Chanyang <124057018+woojucy@users.noreply.github.com> Date: Sun, 2 Mar 2025 03:02:39 +0900 Subject: [PATCH 08/11] Update poly/src/polynomial/univariate/sparse.rs Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com> --- poly/src/polynomial/univariate/sparse.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index c3329677f..8a3519f2c 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -247,6 +247,12 @@ pub enum CoeffVecError { DuplicateDegree, } +#[derive(Debug)] +pub enum CoeffVecError { + ZeroCoefficient, + DuplicateDegree, +} + impl SparsePolynomial { // Function to validate that the input coefficient vector is simplified From 91526ec1826d22814171557063ba0eb0a77a0b48 Mon Sep 17 00:00:00 2001 From: woojucy Date: Sun, 2 Mar 2025 03:12:39 +0900 Subject: [PATCH 09/11] Add tests for is_valid_coefficients_vec --- poly/src/polynomial/univariate/sparse.rs | 44 ++++++++++++++---------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index 8a3519f2c..783ec18b6 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -235,24 +235,6 @@ pub enum CoeffVecError { DuplicateDegree, } -#[derive(Debug)] -pub enum CoeffVecError { - ZeroCoefficient, - DuplicateDegree, -} - -#[derive(Debug)] -pub enum CoeffVecError { - ZeroCoefficient, - DuplicateDegree, -} - -#[derive(Debug)] -pub enum CoeffVecError { - ZeroCoefficient, - DuplicateDegree, -} - impl SparsePolynomial { // Function to validate that the input coefficient vector is simplified @@ -366,7 +348,7 @@ impl From> for SparsePolynomial { mod tests { use crate::{ polynomial::Polynomial, - univariate::{DensePolynomial, SparsePolynomial}, + univariate::{sparse::CoeffVecError, DensePolynomial, SparsePolynomial}, EvaluationDomain, GeneralEvaluationDomain, }; use ark_ff::{UniformRand, Zero}; @@ -387,6 +369,30 @@ mod tests { SparsePolynomial::from_coefficients_vec(coeffs) } + #[test] + fn test_valid_coefficients() { + let coeffs = vec![(1, Fr::from(2)), (2, Fr::from(3)), (3, Fr::from(4))]; + let validation = SparsePolynomial::is_valid_coefficients_vec(&coeffs); + assert!(validation.is_ok()); + + let poly = SparsePolynomial::from_coefficients_vec(coeffs); + assert_eq!(poly.coeffs.len(), 3); + } + + #[test] + fn test_invalid_zero_coefficient() { + let coeffs = vec![(1, Fr::from(0)), (2, Fr::from(3))]; + let validation = SparsePolynomial::is_valid_coefficients_vec(&coeffs); + assert!(matches!(validation, Err(CoeffVecError::ZeroCoefficient))); + } + + #[test] + fn test_invalid_duplicate_degrees() { + let coeffs = vec![(1, Fr::from(2)), (1, Fr::from(3))]; + let validation = SparsePolynomial::is_valid_coefficients_vec(&coeffs); + assert!(matches!(validation, Err(CoeffVecError::DuplicateDegree))); + } + #[test] fn evaluate_at_point() { let mut rng = test_rng(); From ca42a3c867345ceead358fc09db6f271dee07250 Mon Sep 17 00:00:00 2001 From: woojucy Date: Sun, 2 Mar 2025 03:35:21 +0900 Subject: [PATCH 10/11] Add sorting support to is_valid_coefficients_vec --- poly/src/polynomial/univariate/sparse.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index 783ec18b6..2fe2b44d0 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -238,9 +238,11 @@ pub enum CoeffVecError { impl SparsePolynomial { // Function to validate that the input coefficient vector is simplified - pub fn is_valid_coefficients_vec(coeffs: &[(usize, F)]) -> Result<(), CoeffVecError> { + pub fn is_valid_coefficients_vec(coeffs: &mut [(usize, F)]) -> Result<(), CoeffVecError> { + coeffs.sort_by(|a, b| a.0.cmp(&b.0)); + let mut last_degree = None; - for &(degree, ref coeff) in coeffs { + for &mut (degree, ref mut coeff) in coeffs { if coeff.is_zero() { // zero term has no effect return Err(CoeffVecError::ZeroCoefficient); @@ -371,8 +373,8 @@ mod tests { #[test] fn test_valid_coefficients() { - let coeffs = vec![(1, Fr::from(2)), (2, Fr::from(3)), (3, Fr::from(4))]; - let validation = SparsePolynomial::is_valid_coefficients_vec(&coeffs); + let mut coeffs = vec![(1, Fr::from(2)), (2, Fr::from(3)), (3, Fr::from(4))]; + let validation = SparsePolynomial::is_valid_coefficients_vec(&mut coeffs); assert!(validation.is_ok()); let poly = SparsePolynomial::from_coefficients_vec(coeffs); @@ -381,15 +383,15 @@ mod tests { #[test] fn test_invalid_zero_coefficient() { - let coeffs = vec![(1, Fr::from(0)), (2, Fr::from(3))]; - let validation = SparsePolynomial::is_valid_coefficients_vec(&coeffs); + let mut coeffs = vec![(1, Fr::from(0)), (2, Fr::from(3))]; + let validation = SparsePolynomial::is_valid_coefficients_vec(&mut coeffs); assert!(matches!(validation, Err(CoeffVecError::ZeroCoefficient))); } #[test] fn test_invalid_duplicate_degrees() { - let coeffs = vec![(1, Fr::from(2)), (1, Fr::from(3))]; - let validation = SparsePolynomial::is_valid_coefficients_vec(&coeffs); + let mut coeffs = vec![(1, Fr::from(2)), (1, Fr::from(3))]; + let validation = SparsePolynomial::is_valid_coefficients_vec(&mut coeffs); assert!(matches!(validation, Err(CoeffVecError::DuplicateDegree))); } From 06bfae557d563f22aef898dae3d1ef34e97737d5 Mon Sep 17 00:00:00 2001 From: woojucy Date: Wed, 12 Mar 2025 21:20:06 +0900 Subject: [PATCH 11/11] Resolve merge conflicts --- poly/src/polynomial/univariate/sparse.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/poly/src/polynomial/univariate/sparse.rs b/poly/src/polynomial/univariate/sparse.rs index e7402a9cb..25459f861 100644 --- a/poly/src/polynomial/univariate/sparse.rs +++ b/poly/src/polynomial/univariate/sparse.rs @@ -236,7 +236,6 @@ pub enum CoeffVecError { } impl SparsePolynomial { - // Function to validate that the input coefficient vector is simplified pub fn is_valid_coefficients_vec(coeffs: &mut [(usize, F)]) -> Result<(), CoeffVecError> { coeffs.sort_by(|a, b| a.0.cmp(&b.0)); @@ -257,7 +256,7 @@ impl SparsePolynomial { } Ok(()) } - + /// Constructs a new polynomial from a list of coefficients. pub fn from_coefficients_slice(coeffs: &[(usize, F)]) -> Self { Self::from_coefficients_vec(coeffs.to_vec())