Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Arrabiata: prepare constraints for cross-terms computation #2702
base: master
Are you sure you want to change the base?
Arrabiata: prepare constraints for cross-terms computation #2702
Changes from 7 commits
c28b4d4
3c2334b
3864abb
687e400
1e9e66c
d60a681
d3fe0ba
cf76131
28b4d66
98cdfd9
44a3f08
ba0fad3
c23758f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A way to make this test test thing would be to checked the cross term property
poly(a +r*b) = poly(a) + r^n poly(b) + \sum_i r^i cross_term_i(a,b)
Also a nit: I wouldn't call that an integration test
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.
That's the reason "integration" is double quoted.
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.
See the comment just above:
Instead of having a power, we do consider them as a different variable.
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.
See https://github.com/o1-labs/proof-systems/pull/2702/files#r1820537584
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.
is this an invariant the caller needs to respect ?
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.
I m not sure I understand what is computed here ?
the text mentioned combining the polys with one combiner, and we have two of them here.
I understand that the two combiners are the alpha from each instances we fold ?
But then I don't get the assymetrical role ?
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.
See https://hackmd.io/@dannywillems/Syo5MBq90#Computation-of-cross-terms-for-shifted-polynomials
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.
I would change the name of the p variable
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.
p
is forpower
, which is the power ofr
.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.
isn't combiner a random term that cannot be zero ?
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.
Yes, but it was monstly for the tests (see test directory). I didn't handle it initially. Even if the case is negligible, I think it is ok to have it. Maybe we should fail instead?
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.
I'd ignore it to simplify the code, and remove the testing with zero, but do as you feel