Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use polynomial.rs instead of virtual_poly for equality polynomial #42

Merged
merged 17 commits into from
Aug 2, 2023
Merged

Use polynomial.rs instead of virtual_poly for equality polynomial #42

merged 17 commits into from
Aug 2, 2023

Conversation

oskarth
Copy link
Collaborator

@oskarth oskarth commented Jul 20, 2023 •
edited
Loading

  • Remove custom eq_poly and use EqPolynomial instead
  • Update polynomial docs
  • Ensures order of evaluation is consistent between for VirtualPoly and EqPolynomial
  • Remove old build_eq_x_r_vec and build_eq_x_r_helper
  • Tests around boolean hypercube and other introduced functions to make endianness explicit etc

Addresses #19

@oskarth oskarth requested review from arnaucube and CPerezz July 20, 2023 05:40
@oskarth
Copy link
Collaborator Author

oskarth commented Jul 20, 2023

Realized I forgot build_eq_x_r, didn't see in my notes initially

@oskarth oskarth marked this pull request as draft July 20, 2023 06:33
@oskarth oskarth force-pushed the refactor/eq-poly branch from e7c9aaa to 58d0873 Compare July 20, 2023 09:01
@CPerezz
Copy link
Member

CPerezz commented Jul 20, 2023

Isn't this ready for review yet @oskarth ?

@oskarth oskarth marked this pull request as ready for review July 20, 2023 10:47
@oskarth
Copy link
Collaborator Author

oskarth commented Jul 20, 2023

@CPerezz it can be merged as is, I can do the build_eq_x_r in a separate PR. Had some failing tests (related to MLE stuff), will look into more tomorrow

@oskarth oskarth force-pushed the refactor/eq-poly branch from 1a6423b to bc43637 Compare July 22, 2023 05:02
@oskarth oskarth changed the title Refactor/eq poly WIP: Use polynomial.rs instead of virtual_poly for equality polynomial Jul 22, 2023
@oskarth
Copy link
Collaborator Author

oskarth commented Jul 22, 2023 •
edited
Loading

A bit stuck. Tests currently failing due to different order of evaluations inside the MLE. E.g:

[src/ccs/util/virtual_poly.rs:576] eq_x_r_old.Z.clone() = [
    0x34a2da9a5f67ca3e8955b20a35d0bb5e33f182e4821c672b7b60bee92cc210ae,
    0x1a4de95e591506758747746c9f5a8b82926547116f590b455add73e40043959d,
    0x1baaba6f39ed721fed5d82f989195a505a3990fa4657c770b059eb68e1efcb3d,
    0x156481980d95bd2c0205568fa1bb5ecf23fcd707db5c17d991f5b80bf10a8e7b,
]
[src/ccs/util/virtual_poly.rs:577] eq_x_r_new.Z.clone() = [
    0x34a2da9a5f67ca3e8955b20a35d0bb5e33f182e4821c672b7b60bee92cc210ae,
    0x1baaba6f39ed721fed5d82f989195a505a3990fa4657c770b059eb68e1efcb3d,
    0x1a4de95e591506758747746c9f5a8b82926547116f590b455add73e40043959d,
    0x156481980d95bd2c0205568fa1bb5ecf23fcd707db5c17d991f5b80bf10a8e7b,
]

Existing test failing: cargo test test_compute_g

Minimal reproducible test for ordering: cargo test test_eq_x_r_equality

See this commit 567aa34 for failing diff and how to reproduce.

@oskarth oskarth force-pushed the refactor/eq-poly branch 2 times, most recently from 567aa34 to 1cc96fb Compare July 24, 2023 03:54
@oskarth oskarth changed the title WIP: Use polynomial.rs instead of virtual_poly for equality polynomial Use polynomial.rs instead of virtual_poly for equality polynomial Jul 27, 2023
@oskarth
Copy link
Collaborator Author

oskarth commented Jul 27, 2023

@CPerezz @arnaucube ready for review

CPerezz
CPerezz reviewed Jul 31, 2023
View reviewed changes
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Some comments but overall looks good

src/ccs/multifolding.rs Outdated
Comment on lines 79 to 128
let e1 = EqPolynomial::new(r_x.to_vec()).evaluate(r_x_prime);
let e2 = EqPolynomial::new(beta.to_vec()).evaluate(r_x_prime);
Copy link
Member

Choose a reason for hiding this comment

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

The to_vec forces the clone.
Can't we modify the fn signature then so that forces us to clone too? (So no reference)?? Otherwise, this looks like you just need a ref while in reality, is basically copying the entire vector inside.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I'd rather not change Nova code API (EqPolynomial::new args) in this PR tbh unless strong need, since it touches upstream and other stuff

oskarth added 14 commits August 1, 2023 14:13
@oskarth
refactor: Modify polynomial module to be public
8014c0c 
Including EqPolynomial. This ensures we get docstrings for this module.
@oskarth
docs: Add documentation for new MultilinearPolynomial constructor and…
96fe38c 
… methods
@oskarth
docs: Update documentation for EqPolynomial
1cf0320
@oskarth
refactor(multifolding): Use EqPolynomial
ebad146 
Remove old eq_eval, redundant and probably less performant.
@oskarth
docs: Add is_empty method to MultilinearPolynomial class
d4e2334 
Fixes clippy error
@oskarth
WIP: Use Spartan polynomial.rs, not virtual_poly
960ce2e 
Tests currently failing due to different order of evaluations inside the MLE. E.g:

```
[src/ccs/util/virtual_poly.rs:576] eq_x_r_old.Z.clone() = [
    0x34a2da9a5f67ca3e8955b20a35d0bb5e33f182e4821c672b7b60bee92cc210ae,
    0x1a4de95e591506758747746c9f5a8b82926547116f590b455add73e40043959d,
    0x1baaba6f39ed721fed5d82f989195a505a3990fa4657c770b059eb68e1efcb3d,
    0x156481980d95bd2c0205568fa1bb5ecf23fcd707db5c17d991f5b80bf10a8e7b,
]
[src/ccs/util/virtual_poly.rs:577] eq_x_r_new.Z.clone() = [
    0x34a2da9a5f67ca3e8955b20a35d0bb5e33f182e4821c672b7b60bee92cc210ae,
    0x1baaba6f39ed721fed5d82f989195a505a3990fa4657c770b059eb68e1efcb3d,
    0x1a4de95e591506758747746c9f5a8b82926547116f590b455add73e40043959d,
    0x156481980d95bd2c0205568fa1bb5ecf23fcd707db5c17d991f5b80bf10a8e7b,
]
```

Existing test failing: `cargo test test_compute_g`

Minimal reproducible test for ordering: `cargo test test_eq_x_r_equality`
@oskarth
refactor: Rename evaluate_at to evaluate_at_little and add evaluate_a…
d3b431f 
…t_big
@oskarth
XXX: Comment out old eq_x_r stuff
7bd32f1
@oskarth
Revert "XXX: Comment out old eq_x_r stuff"
6632d25 
This reverts commit a8b242d.
@oskarth
XXX: Try to change polynomial.rs
2f45bff 
Just to see how changing order of evals there impacts tests

With this minimal equality test succeeds but integration test fails
@oskarth
Revert "XXX: Try to change polynomial.rs"
204c9fa 
This reverts commit 9a9895b.
@oskarth
fix: Reorder evaluations to match endianness of VirtualPoly
b77cb90 
Tests pass with this
@oskarth
Cleanup
9c761bd
@oskarth
Update docs
e62b5d2
@oskarth oskarth requested a review from CPerezz August 1, 2023 12:32
@oskarth oskarth mentioned this pull request Aug 1, 2023
Open
CPerezz
CPerezz approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM

@oskarth oskarth merged commit bb7a651 into main Aug 2, 2023
@CPerezz CPerezz deleted the refactor/eq-poly branch August 2, 2023 08:13
This was referenced Aug 2, 2023
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CPerezz CPerezz CPerezz approved these changes

@arnaucube arnaucube Awaiting requested review from arnaucube

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oskarth @CPerezz