-
Notifications
You must be signed in to change notification settings - Fork 260
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
Implement Polynomial
for MultilinearExtension
#691
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
82cc282
Implement Polynomial trait for MultilinearExtensions
mmagician 30dce56
restricted Point associated type to Vec<F> for MLE
Antonio95 183a13b
Merge branch 'master' into ml-is-poly-vec
mmagician e4690c4
Add CHANGELOG entry
mmagician 25f68b3
Merge branch 'master' into ml-is-poly-vec
mmagician f8e0378
Disambiguate the examples in poly/README
mmagician 0ab3d75
Merge remote-tracking branch 'HungryCatsStudio/ml-is-poly-vec' into m…
mmagician 14eb52a
remove evaluate from MultilinearExtension
mmagician 8c28679
fmt
mmagician 678ad2f
fix bench imports
mmagician 0df1bd9
change degree to represent total_degree
mmagician File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
It seems that elsewhere in the crate, we assume that
degree
here represents total degree, and not individual degree. This is in agreement with mathematical convention.So this should be changed to return
self.num_vars
, and it might be worth adding anindividual_degree
method toPolynomial
that would return the degree for univariate polynomials,1
for multilinear polynomials, and the actual individual degree for general multivariate 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 adapted this to represent the total degree. I've also created a separate issue to discuss the exact design and the need for
individual_degree
: #692.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 method meant to represent a total degree bound, or the actual total degree of the particular instance this is being called on? For instance, the polynomial
f(x, y, z) = xy + yz
hasnum_vars
equal to 3, but actual total degree 2.Returning the actual total degree would be simple in the sparse case, but might require some work in the dense case if the polynomial is stored in evaluation form.
Incidentally, this caveat would also have applied to the code as it was before @Pratyush 's comment: we were always returning 1, but in the fringe case of constant polynomials, that did not match the total degree.