-
Notifications
You must be signed in to change notification settings - Fork 79
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.
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
Add interaction_element_ids to Component trait. #631
Add interaction_element_ids to Component trait. #631
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #631 +/- ##
==========================================
- Coverage 92.64% 92.52% -0.12%
==========================================
Files 69 69
Lines 8995 9006 +11
Branches 8995 9006 +11
==========================================
Hits 8333 8333
- Misses 590 601 +11
Partials 72 72 ☔ View full report in Codecov by Sentry. |
c2d33be
to
63c2304
Compare
a8fa06c
to
230a4ef
Compare
63c2304
to
2eec1f7
Compare
230a4ef
to
7131f61
Compare
7131f61
to
1d679cd
Compare
1d679cd
to
b3619fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5 and @andrewmilson)
crates/prover/src/core/mod.rs
line 63 at r1 (raw file):
} pub struct InteractionElements(Vec<(String, BaseField)>);
This should be SecureField
Code quote:
BaseField
crates/prover/src/core/air/air_ext.rs
line 53 at r1 (raw file):
.dedup() .map(|id| { let element = channel.draw_felt().0 .0;
Consider to draw all the elements together.
I think we have draw_felts
function
Each channel hash could give you more then one element
Code quote:
channel.draw_felt().0
b3619fe
to
a9469cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)
crates/prover/src/core/mod.rs
line 63 at r1 (raw file):
Previously, shaharsamocha7 wrote…
This should be SecureField
Right. I did this on purpose to add a constraint first more easily, and I plan to change it to a SecureField on top of this stack (there's also a TODO for it).
crates/prover/src/core/air/air_ext.rs
line 53 at r1 (raw file):
Previously, shaharsamocha7 wrote…
Consider to draw all the elements together.
I think we havedraw_felts
function
Each channel hash could give you more then one element
Done.
a9469cf
to
49b2acb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5)
crates/prover/src/core/mod.rs
line 63 at r3 (raw file):
} pub struct InteractionElements(Vec<(String, BaseField)>);
Shouldn't this be SecureField?
Code quote:
BaseField
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson)
crates/prover/src/core/mod.rs
line 63 at r3 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
Shouldn't this be SecureField?
Right. I did this on purpose to add a constraint first more easily, and I plan to change it to a SecureField on top of this stack (there's also a TODO for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5)
crates/prover/src/core/air/mod.rs
line 44 at r3 (raw file):
) -> ColumnVec<Vec<CirclePoint<SecureField>>>; // Returns the ids of the interaction elements used by the component.
Suggestion:
///
49b2acb
to
ab4f4c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson)
crates/prover/src/core/air/mod.rs
line 44 at r3 (raw file):
) -> ColumnVec<Vec<CirclePoint<SecureField>>>; // Returns the ids of the interaction elements used by the component.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 6 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @shaharsamocha7)
crates/prover/src/core/air/air_ext.rs
line 55 at r4 (raw file):
let elements = channel.draw_felts(ids.len()).into_iter().map(|e| e.0 .0); InteractionElements(zip_eq(ids, elements).collect_vec()) }
Optional: could use BTreeSet<String>
for interaction_element_ids
Suggestion:
fn interaction_elements(&self, channel: &mut Blake2sChannel) -> InteractionElements {
let mut ids = BTreeSet::new();
for component in self.components() {
ids = &ids | &component.interaction_element_ids();
}
let elements = channel.draw_felts(ids.len()).into_iter().map(|e| e.0 .0);
InteractionElements(zip_eq(ids, elements).collect_vec())
}
ab4f4c1
to
a976f53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @andrewmilson and @shaharsamocha7)
crates/prover/src/core/air/air_ext.rs
line 55 at r4 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
Optional: could use
BTreeSet<String>
for interaction_element_ids
Done.
This change is