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

Correct misleading comment about the choice of polynomial #297

Merged
merged 5 commits into from Dec 16, 2022

Conversation

intoverflow
Copy link
Member

Also adds a unit test to pin-down the choice of field extension.

…est to pin-down the choice of field extension.
@jacobdweightman
Copy link

There are also a few (duplicated) comments which mention X^4 - 11 in zirgen. I'm fine with this as-is, but I would also like to see those fixed.

@pdg744
Copy link
Contributor

pdg744 commented Dec 6, 2022

subtle nit:

I noticed that in addition to changing - 11 to + 11, this PR also changes - BETA to + BETA. It looks like BETA appears in code, and it's not quite clear to me whether we're using:

f(x) = x - BETA, where BETA = -11

f(x) = x + BETA where BETA = 11`.

@jacobdweightman @jbruestle maybe one of you can clarify?

@pdg744
Copy link
Contributor

pdg744 commented Dec 6, 2022

i.e. see BETA vs. NBETA here

const BETA: Elem = Elem::new(11);

@pdg744
Copy link
Contributor

pdg744 commented Dec 6, 2022

I think we can ignore my nit above: I realized after writing this that "NBETA" almost certainly means "negative BETA," which seems to clarify that BETA = 11 and the polynomial is f(x) = x + BETA.

@intoverflow intoverflow enabled auto-merge (squash) December 16, 2022 15:06
@intoverflow intoverflow merged commit 6417c6f into main Dec 16, 2022
@intoverflow intoverflow deleted the tcarstens/bb branch December 16, 2022 15:48
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.

None yet

4 participants