Skip to content

Add FieldVar::inner_product and Sum bounds on FieldVar#173

Merged
Pratyush merged 2 commits intomasterfrom
inner-product-fp
May 30, 2025
Merged

Add FieldVar::inner_product and Sum bounds on FieldVar#173
Pratyush merged 2 commits intomasterfrom
inner-product-fp

Conversation

@Pratyush
Copy link
Copy Markdown
Member

@Pratyush Pratyush commented May 30, 2025

Description

Also fixes #151.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@Pratyush Pratyush requested a review from a team as a code owner May 30, 2025 19:15
@Pratyush Pratyush requested review from alireza-shirzad, Copilot, mmagician, weikengchen and z-tech and removed request for a team May 30, 2025 19:15
@Pratyush Pratyush enabled auto-merge May 30, 2025 19:16
Copy link
Copy Markdown
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 enhances the FieldVar abstraction by adding a generic inner_product method and Sum trait support across various field variable types, enabling concise summation and dot-product operations in R1CS.

  • Extended FieldVar trait with inner_product and Sum bounds
  • Implemented Sum for QuadExtVar, CubicExtVar, and EmulatedFpVar
  • Added optimized inner_product and batch-sum helpers in AllocatedFp and FpVar

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/fields/quadratic_extension.rs Added Sum impls for QuadExtVar
src/fields/mod.rs Updated FieldVar trait with inner_product and Sum
src/fields/fp/mod.rs Added add_many, linear_combination, and inner_product to AllocatedFp; updated FpVar uses
src/fields/emulated_fp/field_var.rs Added Sum impls for EmulatedFpVar
src/fields/cubic_extension.rs Added Sum impls for CubicExtVar
Cargo.toml Introduced itertools dependency
Comments suppressed due to low confidence (1)

src/fields/quadratic_extension.rs:562

  • [nitpick] There are no unit tests covering the new Sum impl for QuadExtVar or the inner_product on FieldVar. Consider adding tests to validate these behaviors.
impl<BF, P> Sum<Self> for QuadExtVar<BF, P>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Pratyush Pratyush disabled auto-merge May 30, 2025 19:21
@Pratyush Pratyush requested a review from Copilot May 30, 2025 19:21
Copy link
Copy Markdown
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 adds generic summation and inner-product capabilities to field variables, enabling dot products and collection-based sums across various field extensions.

  • Extends FieldVar trait with Sum bounds and a default inner_product method.
  • Implements Sum for QuadExtVar, CubicExtVar, and EmulatedFpVar.
  • Provides specialized inner_product and improved aggregation in the fp module, updates AllocatedFp::add_many, and adds tests for FpVar.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/fields/mod.rs Added Sum bounds and default inner_product to FieldVar.
src/fields/quadratic_extension.rs Implemented Sum<Self> and Sum<&Self> for QuadExtVar.
src/fields/cubic_extension.rs Implemented Sum<Self> and Sum<&Self> for CubicExtVar.
src/fields/fp/mod.rs Updated add_many, added linear_combination, specialized inner_product and Sum for FpVar.
src/fields/emulated_fp/field_var.rs Added Sum impls for EmulatedFpVar.
Cargo.toml Added itertools dependency to support multiunzip and zip_eq.
Comments suppressed due to low confidence (2)

src/fields/cubic_extension.rs:589

  • [nitpick] Similarly, this collects three vectors, which may hurt performance. Consider a single-pass fold to accumulate c0, c1, and c2 without intermediate allocations.
let (c0s, c1s, c2s): (Vec<_>, Vec<_>, Vec<_>) = itertools::multiunzip(iter.map(|x| (x.c0, x.c1, x.c2)));

src/fields/quadratic_extension.rs:562

  • [nitpick] There are currently no unit tests for the new Sum and inner_product implementations on QuadExtVar (and similarly for CubicExtVar). Adding tests for these will help catch future regressions.
impl<BF, P> Sum<Self> for QuadExtVar<BF, P>

@Pratyush Pratyush enabled auto-merge May 30, 2025 19:26
@Pratyush Pratyush added this pull request to the merge queue May 30, 2025
Merged via the queue into master with commit b7ab5cc May 30, 2025
11 checks passed
@Pratyush Pratyush deleted the inner-product-fp branch May 30, 2025 19:39
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.

add_many panics with an empty iterator

3 participants