-
Notifications
You must be signed in to change notification settings - Fork 132
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
Fix default check combinations #123
Conversation
Hmm I might have messed something up, I converted to draft for now |
fix combinations remove prints remove old comments
…lin_pc" This reverts commit 25d4c8c.
b2fdf43
to
a209c8c
Compare
Ok, I was a bit rash on squashing two loops into one - it turns out we need to first collect before zipping to ensure correct ordering by the new key. Ready now ✅ |
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.
LGTM modulo the nits.
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
iter -> into_iter
By the way @mmagician, in a separate PR, do you mind adding comments explaining what this code is doing? (Since you touched this code most recently you have the best idea.) |
Description
The trait-default implementation of
check_combinations
was incorrect. This was not detected because all of the implemented schemes have a custom implementation of this method. We are going to use the default method in Ligero though.Description of the bug(s):
poly_evals: BTreeMap
was of type(query_label, point)
, whereas the first element in the key tuple should have been a poly_label. This was causing map access here to fail withMissingEvaluation
.poly_query_set
was keyed only withpoly_label
, but held multiple query values with the samepoly_label
. Iterating over its keys was be done in an arbitrary order.evals
, is returned callingvalues()
onBTreeMap
, the values are returned sorted by key of type(poly_label, point)
.poly_label, point
), and collect first, so before it gets combined with a sorted vector of evaluations, it will itself get sorted on the correct key(poly_label, point)
.(2 might seem obvious in retrospect)
As mentioned, this is impossible to validate against the current codebase where all PCS have a custom implementation. To showcase the bug, I've sandwiched the actual fix in between 2 commits, where I first remove the custom impl from
marlin_pc
and later add it back after the fix. To verify the fix, you can check out 57ac1ab, and runcargo test marlin::marlin_pc::tests::full_end_to_end_equation_test
-> should fail.Then checkout the fix commit dcffaec, run the same command, and verify the tests pass now.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
Files changed
in the Github PR explorer