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

Fix bench Mul128 error #150

Merged
merged 6 commits into from
Nov 6, 2023
Merged

Fix bench Mul128 error #150

merged 6 commits into from
Nov 6, 2023

Conversation

jmjac
Copy link
Contributor

@jmjac jmjac commented Nov 2, 2023

Benchmark for Mul128 was failing when randomFeltElement was bigger than u128. This should fix it. Also there is (&f.Element{}).SetRandom() to create felts between [0,q). I think we could use it instead of current randomFeltElement implementation unless I'm missing something.

@jmjac jmjac requested a review from rodrigo-pino November 2, 2023 10:32
@rodrigo-pino
Copy link
Contributor

Benchmark for Mul128 was failing when randomFeltElement was bigger than u128. This should fix it. Also there is (&f.Element{}).SetRandom() to create felts between [0,q). I think we could use it instead of current randomFeltElement implementation unless I'm missing something.

@jmjac the reason behind not using it is that I was looking for certain determinism in benchmark test. There are lot of variables during benchmarks already, just to make sure we are testing the same thing. I've also noticed that randomFeltElement is not entirely correct since it could output invalid felt numbers, something to fix in the near future

@jmjac
Copy link
Contributor Author

jmjac commented Nov 6, 2023

Makes sense. For the randomFeltElement we could use the similar approach as for u128 and limit the last z[3] to 50bits like

v = binary.BigEndian.AppendUint64(nil, rand.Uint64()>>14)
copy(b[0:8], v)

so that at max we would be using 250bits which is lower than 251 bits of felt.

@rodrigo-pino
Copy link
Contributor

so that at max we would be using 250bits which is lower than 251 bits of felt

I agree, would you like to add this fix as part of this PR?

@jmjac
Copy link
Contributor Author

jmjac commented Nov 6, 2023

@rodrigo-pino added the fix and simplified the function

@rodrigo-pino rodrigo-pino merged commit f93b6d1 into main Nov 6, 2023
4 checks passed
@rodrigo-pino rodrigo-pino deleted the fix-benchmarks branch November 6, 2023 16:08
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.

2 participants