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

Implement batch_inverse on M31,CM31 #137

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented Oct 27, 2024

This change is Reviewable

Copy link
Contributor Author

andrewmilson commented Oct 27, 2024

@andrewmilson andrewmilson force-pushed the 10-23-Optimize_Coset_at branch 2 times, most recently from 88b78cd to b3d6d74 Compare October 29, 2024 03:34
@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch from 8f80f3b to 879c0ba Compare October 29, 2024 03:34
@andrewmilson andrewmilson force-pushed the 10-23-Optimize_Coset_at branch from b3d6d74 to f015e1a Compare November 6, 2024 03:15
@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch from 879c0ba to e8e2b31 Compare November 6, 2024 03:16
@andrewmilson andrewmilson force-pushed the 10-23-Optimize_Coset_at branch from f015e1a to 7a97fdb Compare November 12, 2024 21:01
@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch 2 times, most recently from 82903f4 to 5307458 Compare November 12, 2024 21:24
@andrewmilson andrewmilson marked this pull request as ready for review November 12, 2024 21:25
@andrewmilson andrewmilson force-pushed the 10-23-Optimize_Coset_at branch from 943c4da to b70127f Compare November 14, 2024 14:15
@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch from 4bce875 to a58d998 Compare November 14, 2024 14:15
@andrewmilson andrewmilson force-pushed the 10-23-Optimize_Coset_at branch from b70127f to 9c29983 Compare November 14, 2024 18:43
@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch from a58d998 to a74d6cd Compare November 14, 2024 18:43
Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

I'm just not sure weather it's worth the effort to add libfunc or modify the proof because the batch inverses only account for ~3% of the total step count in the verification benchmarks I ran. WDYT?

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

Though maybe I'm overestimating the effort to add a libfunc or modify the proof.

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

@andrewmilson andrewmilson force-pushed the 10-23-Optimize_Coset_at branch from 9c29983 to aab4ad6 Compare November 17, 2024 16:19
@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch from a74d6cd to 45c1e13 Compare November 17, 2024 16:19
@andrewmilson andrewmilson force-pushed the 10-23-Optimize_Coset_at branch from aab4ad6 to eec00b8 Compare November 17, 2024 18:09
@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch from 45c1e13 to 729aacf Compare November 17, 2024 18:09
Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 9 files at r1.
Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

@andrewmilson andrewmilson force-pushed the 10-23-Optimize_Coset_at branch from eec00b8 to fa29a42 Compare November 19, 2024 14:40
Base automatically changed from 10-23-Optimize_Coset_at to main November 19, 2024 14:44
@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch 2 times, most recently from 44331ce to f29ed97 Compare November 19, 2024 15:44
Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)


stwo_cairo_verifier/src/fields.cairo line 8 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I'd reserve Field for something else.

Done.


stwo_cairo_verifier/src/fields.cairo line 18 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I reported this one, but in the future, please report formatting errors in #compiler-dev.

Done. tyty


stwo_cairo_verifier/src/fields.cairo line 25 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

use pop_back

Done.


stwo_cairo_verifier/src/fields.cairo line 28 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

while let Some(value) = values_span.pop_back()

Done.

Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 8 files at r4.
Reviewable status: 6 of 9 files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@andrewmilson andrewmilson force-pushed the 10-24-Implement_batch_inverse_on_M31_CM31 branch from f29ed97 to 643b60a Compare November 19, 2024 15:47
@andrewmilson andrewmilson merged commit 61ee03b into main Nov 19, 2024
5 checks passed
@andrewmilson andrewmilson deleted the 10-24-Implement_batch_inverse_on_M31_CM31 branch November 19, 2024 15:51
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