-
Notifications
You must be signed in to change notification settings - Fork 200
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
Include Lagrange kernel trace polynomial term into DEEP composition polynomial #274
Conversation
d0c73e1
to
e6bc2b4
Compare
Rebased and updated the code in light of #271 |
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! Not a full review yet - but I left some small comments/questions inline.
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 more small comments inline.
Perhaps the biggest thing is the treatment of auxiliary trace width, related to this, the position of the Lagrange kernel column in the auxiliary trace. Basically, it is not immediately clear to me if we've correctly reduce the auxiliary trace width everywhere. I think the solution here would be to remove the ability to specify Lagrange kernel column index manually (and also handle construction of the Lagrange column within the library) - but this is for a different PR.
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 of comments above. I think the main ones are:
- Move the new division benchmark into the
math
crate. - Create an issue for investigating FFT-based division in the future.
As the title says, this PR includes the missing term(s) in the DEEP composition polynomial related to the Lagrange kernel trace polynomial. A few remarks are in order: