Skip to content
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

Refactoring: Improve poly-commitment docs & decrease confusing variable names #2811

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

volhovm
Copy link
Member

@volhovm volhovm commented Nov 20, 2024

This is a no-op docs/naming refactoring commit that does the following:

  • Moves combine_polys to poly-commitment/src/utils.rs (there was a comment left by someone that this is a good idea; I agree)
  • Improves docs for combine_polys and related structs, like PolynomialsToCombine.
  • Renames some variables inside combine_polys: "omegas" was used which is (1) only used once in the whole repo, and (2) is absolutely not descriptive, since it has no r. Was replaced by
  • Fixed some typos/bad argument names for hiding/non_hiding functions, that are actually operating over the integer expressing the number of chunks, not the size of the domain as the previous variable implied.
  • Importantly: gets rid of "xi", which was a very rarely used term to call polyscale, also known as u (not to be confused with the three other us int he code). Used polyscale instead.
    • xi is used more on the ocaml side, but my plan is to remove it from there too. See the justification below.
  • Improves docs/disambiguates variable names in IPA commitment creation and verification.

Parent mina PR:

On xi and justification for not using it:

  • Throughout the codebase, we use xi = polyscale = v and r = evalscale = u which is a /pain/ for someone trying to make sense of what's happening. What's even worse, the term u means at least two other things: a generator in the IPA procedure, and the IPA challenge.
  • I decided to get rid of the xi/r pair of these three, for the following reasons:
    • The original plonk paper uses u and v.
    • Our o1labs book on pickles uses u and v (with references to polyscale and evalscale).
    • Most of proof-systems except some 10 lines uses u/v or polyscale/evalscale.
    • r is a terribly overloaded variable name, which probably means about 5 things across the codebase already (e.g. r in schnorr.ml or signature.ml, e.g. l and r in plonk_constraint_system.ml). So I'm strongly against using it as a global identifier if possible.
    • I think it would be better to stick with globally identifiable handles, like polyscale. I think it's acceptable to keep two notations, one concise-mathematical, one globally-identifiable and code-friendly. So one mathematical notation should go, and xi / r is the most ambiguous of the two and least used.

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.12%. Comparing base (370e099) to head (6542666).
Report is 30 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2811   +/-   ##
=======================================
  Coverage   72.12%   72.12%           
=======================================
  Files         256      257    +1     
  Lines       59942    59968   +26     
=======================================
+ Hits        43231    43252   +21     
- Misses      16711    16716    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant