Skip to content

Implement batch_inverse_and_mul - #100

Merged
Pratyush merged 9 commits into
masterfrom
batch_inv_and_mul
Nov 28, 2020
Merged

Implement batch_inverse_and_mul#100
Pratyush merged 9 commits into
masterfrom
batch_inv_and_mul

Conversation

@ValarDragon

Copy link
Copy Markdown
Collaborator

Description

Implement batch inverse and mul, and add tests for both batch inverse and batch inverse & mul.

closes: #98


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 (main)
  • 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

Copy link
Copy Markdown
Member

Thanks for the PR! To get the test to compile you need to add a dev-dependency on ark-test-curves.

@ValarDragon

ValarDragon commented Nov 28, 2020

Copy link
Copy Markdown
Collaborator Author

Thanks! Just realized I was testing it incorrectly locally (forgot to use all features)

I'll try to fix the remaining build errors later tonight

@ValarDragon
ValarDragon marked this pull request as draft November 28, 2020 00:16
@ValarDragon

Copy link
Copy Markdown
Collaborator Author

I'm running into an error that doesn't really make sense to me:

484 | pub fn batch_inversion<F: Field>(v: &mut [F]) {
    |                           ----- required by this bound in `fields::batch_inversion`
...
559 |         batch_inversion::<Fr>(&mut random_coeffs_inv);
    |                               ^^^^^^^^^^^^^^^^^^^^^^ the trait `fields::Field` is not implemented for `ark_ff::fields::models::Fp256<FrParameters>`

Not sure how to fix it

@Pratyush

Copy link
Copy Markdown
Member

I'm running into an error that doesn't really make sense to me:

484 | pub fn batch_inversion<F: Field>(v: &mut [F]) {
    |                           ----- required by this bound in `fields::batch_inversion`
...
559 |         batch_inversion::<Fr>(&mut random_coeffs_inv);
    |                               ^^^^^^^^^^^^^^^^^^^^^^ the trait `fields::Field` is not implemented for `ark_ff::fields::models::Fp256<FrParameters>`

Not sure how to fix it

Hmm this is probably because of some circular dependency issues: test-curves is using a different "version" of ff in the dependency tree than ff's tests are. Ugh this is annoying. If you'd like you can just copy paste bls12_381's Fr into a private module of ark-ff (and similarly for ark-ec).

@ValarDragon

Copy link
Copy Markdown
Collaborator Author

Oh cool, that worked!

Comment thread ff/src/lib.rs
ValarDragon and others added 2 commits November 27, 2020 23:13
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
@Pratyush

Copy link
Copy Markdown
Member

@ValarDragon Is this ready to merge?

@ValarDragon

Copy link
Copy Markdown
Collaborator Author

Yes!

@ValarDragon
ValarDragon marked this pull request as ready for review November 28, 2020 05:38
@Pratyush Pratyush changed the title Implement Batch inverse and mul Implement batch_inverse_and_mul Nov 28, 2020
@Pratyush
Pratyush merged commit ae21931 into master Nov 28, 2020
@Pratyush
Pratyush deleted the batch_inv_and_mul branch November 28, 2020 05:40
@ValarDragon

Copy link
Copy Markdown
Collaborator Author

Thanks for the help in debugging this!

@Pratyush

Copy link
Copy Markdown
Member

np, thanks for the PR!

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.

Implement batch inverse and mul

2 participants