-
Notifications
You must be signed in to change notification settings - Fork 195
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 option for algebraic batching to build DEEP polynomial #357
Add option for algebraic batching to build DEEP polynomial #357
Conversation
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.
Looks good! Thank you! I left some comments inline.
Also, shouldn't proof security estimates be affected by the type of batching we do? Or is this too difficult to incorporate in the current structure?
Also, let's resolve the merge conflict against the current |
Exactly! The only thing we need is the the widths of each of the committed traces or the total number of committed columns. If this number is say |
ab9cb5b
to
b995c7f
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.
Looks good! Thank you! I left a couple more comments inline.
Would we do it in this PR? Or should we leave it for a follow-up? |
Some clippy warnings related to |
I propose we do it in a subsequent PR |
I guess these are functionally equivalent to |
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.
Looks good! Thank you! I left some comments inline (mostly about rename BatchType
into BatchMethod
). After these are addressed, we can merge.
We probably need to sync the next branch so that we can change the base branch for this PR.
Once #356 is merged, I can update the security estimator in this PR.