Skip to content

Conversation

@mdqst
Copy link

@mdqst mdqst commented Oct 13, 2025

updated the accumulation from using .sum() to an explicit .fold(F::zero(), |acc, x| acc + x).
this ensures correct behavior for all F: PrimeField types and resolves the issue with summing field elements.

@srinathsetty
Copy link
Collaborator

Can you say more on what the issue with using sum is?

@mdqst
Copy link
Author

mdqst commented Oct 15, 2025

srinathsetty, sum() can be a bit tricky with field types - it goes through the Sum trait, which doesn’t always behave as expected for PrimeField elements (especially with refs and generic bounds). Using an explicit fold(F::zero(), |acc, x| acc + x) makes the accumulation unambiguous and guarantees it stays within the field operations. just safer and clearer

@srinathsetty srinathsetty requested a review from Copilot October 16, 2025 13:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the accumulation logic for PrimeField vectors by replacing .sum() with an explicit .fold(F::zero(), |acc, x| acc + x) operation. This change ensures correct behavior when summing field elements in the sparse matrix-vector multiplication implementation.

Key Changes:

  • Updated the accumulation method in SparseMatrix::multiply to use explicit folding instead of .sum()

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@mdqst
Copy link
Author

mdqst commented Oct 19, 2025

srinathsetty, lets merge it

@srinathsetty
Copy link
Collaborator

srinathsetty, lets merge it

Can we please fix CI failures?

@mdqst
Copy link
Author

mdqst commented Dec 5, 2025

srinathsetty done

@mdqst
Copy link
Author

mdqst commented Dec 25, 2025

srinathsetty lets merge it

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