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

Unify/simplify pickles variable names #16376

Open
wants to merge 3 commits into
base: compatible
Choose a base branch
from

Conversation

volhovm
Copy link
Member

@volhovm volhovm commented Nov 21, 2024

This PR renames certain pickles variables for uniformity. For justification see o1-labs/proof-systems#2811

Concretely:

  • xi (also known as polyscale and u) becomes polyscale
  • r (also known as evalscale and v and randomness) becomes evalscale

This PR is a no-op except for potential serialization backwards compatibility, which I will look into.

@volhovm
Copy link
Member Author

volhovm commented Nov 21, 2024

!ci-build-me

@volhovm
Copy link
Member Author

volhovm commented Nov 21, 2024

!ci-build-me

Copy link
Member

@svv232 svv232 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one comment about an old spelling mistake. I don't think there will be any serialization issues because these wire types don't have a json ppx derived. I'm going to run the nightly in case, but I will approve if that passes.

(** The challenge used for combining polynomials *)
; bulletproof_challenges : 'bulletproof_challenges
(** The challenges from the inner-product argument that was partially verified. *)
; b : 'fq
(** b = challenge_poly plonk.zeta + r * challenge_poly (domain_generrator * plonk.zeta)
(** b = challenge_poly plonk.zeta + evalscale * challenge_poly (domain_generrator * plonk.zeta)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

domain_generator, might as well fix it while we are here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! Should it be generator?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

Copy link
Member Author

@volhovm volhovm Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

domain_generator is still widely used in mina? Or it should also be all renamed into generator?

screen_2024-11-21_000

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no sry i mean that the typo domain_generrator in the comment should be domain_generator instead. I wanted to remove the extra "r"

@svv232
Copy link
Member

svv232 commented Nov 21, 2024

!ci-nightly-me

@coveralls
Copy link

Coverage Status

coverage: 33.188% (-28.9%) from 62.1%
when pulling 37a4f1b on volhovm/rename-pickles-vars
into 422c695 on compatible.

Copy link
Member

@mrmr1993 mrmr1993 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is a no-op except for potential serialization backwards compatibility, which I will look into.

This is my main concern. I think we have to be careful about whether / how the JSON serialisation for o1js etc. may change too, if that's used.

That said, this is great. I've wanted to make this change for years!

@svv232
Copy link
Member

svv232 commented Nov 24, 2024

!ci-nightly-me

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.

4 participants