Skip to content

Fix sparsepoly duplicate terms#954

Open
woojucy wants to merge 12 commits intoarkworks-rs:masterfrom
woojucy:fix_sparsepoly_duplicate_terms
Open

Fix sparsepoly duplicate terms#954
woojucy wants to merge 12 commits intoarkworks-rs:masterfrom
woojucy:fix_sparsepoly_duplicate_terms

Conversation

@woojucy
Copy link

@woojucy woojucy commented Mar 12, 2025

Description

This PR reintroduces the changes from the previously closed PR (#915) to address the issue where duplicate terms with the same degree were not explicitly handled in univariate::SparsePolynomial::from_coefficients_vec. In the current implementation, multiple terms with the same degree remain as separate entries, leading to inconsistencies in polynomial operations and unnecessary redundancy.

To resolve this, an is_valid_coefficients_vec function has been introduced to pre-validate the input vector before constructing a sparse polynomial. This ensures that duplicate terms are merged, preventing redundancy.

Changes Introduced

  • Implemented is_valid_coefficients_vec to check for duplicate terms.
  • Ensured that from_coefficients_vec only processes pre-validated input.
  • Added three test cases to verify the correctness of the validation function.
  • Considered different approaches for handling rand_sparse_poly behavior:
    • Option 1: Integrate the validity check within rand_sparse_poly and implement a retry mechanism.
    • Option 2: Adjust the output based on is_valid_coefficients_vec and modify related test cases accordingly.

Notes

  • The previous PR was automatically closed due to the deletion of the base branch.
  • This PR restores those changes while addressing all previous feedback.
  • Further discussion is welcome regarding how rand_sparse_poly should handle invalid input.

woojucy and others added 11 commits January 4, 2025 20:53
Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com>
Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com>
Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com>
Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com>
Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com>
Co-authored-by: Andrew Zitek-Estrada <1497456+z-tech@users.noreply.github.com>
@woojucy woojucy requested review from a team as code owners March 12, 2025 09:50
@woojucy woojucy requested review from Pratyush, mmagician and weikengchen and removed request for a team March 12, 2025 09:50
@z-tech
Copy link
Contributor

z-tech commented Mar 12, 2025

This got automatically closed when it was based on a branch that was merged and deleted: #915 (comment)

@z-tech
Copy link
Contributor

z-tech commented Mar 12, 2025

@woojucy you need to the merge conflicts otherwise looks good to me

@woojucy
Copy link
Author

woojucy commented Mar 12, 2025

Merge conflicts resolved! Let me know if anything else needs fixing.

@z-tech z-tech self-requested a review March 12, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants