chore: Shared Permutation+Lookup relation arithmetic #559
Conversation
kevaundray
changed the title
zac-williamson
force-pushed
the
zw/shared-permutation-relation-arithmetic
branch
2 times, most recently
from
259d609 to
abf4764
Compare
ledwards2225
requested review from
ledwards2225
and removed request for
codygunton
ledwards2225
force-pushed
the
zw/shared-permutation-relation-arithmetic
branch
from
1fde990 to
d915994
Compare
There was a problem hiding this comment.
Looks good to me. One minor change that I think is worth making: I'd like to remove the keyword "permutation" from objects that refer to arbitrary grand products more generally. For example, I would prefer the function compute_permutation_grand_products (which for Ultra computes both the perm and lookup grand products) be called simply compute_grand_products or perhaps compute_grand_product_polynomials. There are several similar instances.
| // Assign the grand product polynomial to the relevant std::span member of `full_polynomials` (and its shift) | ||
| // For example, for UltraPermutationRelation, this will be `full_polynomials.z_perm` | ||
| // For example, for LookupRelation, this will be `full_polynomials.z_lookup` | ||
| std::span<FF>& full_polynomial = PermutationRelation::get_grand_product_polynomial(full_polynomials); |
There was a problem hiding this comment.
Took me a second to work out what was going on here. The fact that it needed to be done this way seems like an indication that something in our full_polynomials/proving_key model could be improved but nothing really to do with this PR
| ((z_perm_shift + lagrange_last * public_input_delta) * (w_1 + sigma_1 * beta + gamma) * | ||
| (w_2 + sigma_2 * beta + gamma) * (w_3 + sigma_3 * beta + gamma))) * | ||
| (((z_perm + lagrange_first) * | ||
| compute_permutation_numerator<AccumulatorTypes>(input, relation_parameters, 0)) - |
There was a problem hiding this comment.
The index = 0 concerned me at first until I tracked down the different implementations of get_view. It's minor but would be nice to find a way to improve the interface here. Happy to leave as a possible TODO
…polynomial permutation relation arithmetic defined exclusively in relationclasses comments, naming cleanup removed TypeMuncher from relation_types
… being used and not an size_t index
…able`, `unused-parameter` errors.
ledwards2225
force-pushed
the
zw/shared-permutation-relation-arithmetic
branch
from
ca03767 to
6468684
Compare
…arretenberg#559) Co-authored-by: ledwards2225 <[email protected]>
…arretenberg#559) Co-authored-by: ledwards2225 <[email protected]>
Description
For relations that use a grand product, the algebra that defines the relation is contained exclusively within the respective Relation class.
This aligns with how we treat non-grand-product relations and prevents the duplication of complex polynomial algebra (e.g. previously the permutation+grand product algebra was duplicated between the relation class and
prover_library.cppChecklist:
@briefdescribing the intended functionality.includedirectives have been added.