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

Added From<G> in add_constraint. #901

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Added From<G> in add_constraint. #901

merged 1 commit into from
Nov 28, 2024

Conversation

Alon-Ti
Copy link
Contributor

@Alon-Ti Alon-Ti commented Nov 25, 2024

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@Alon-Ti Alon-Ti marked this pull request as ready for review November 25, 2024 11:06
@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.56%. Comparing base (e4c2d1d) to head (9523340).

Additional details and impacted files
@@                 Coverage Diff                  @@
##           alont/simplify-expr     #901   +/-   ##
====================================================
  Coverage                91.56%   91.56%           
====================================================
  Files                       93       93           
  Lines                    13228    13226    -2     
  Branches                 13228    13226    -2     
====================================================
- Hits                     12112    12111    -1     
+ Misses                    1003     1002    -1     
  Partials                   113      113           

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

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 72b28de Previous: cd8b37b Ratio
iffts/simd ifft/22 13490628 ns/iter (± 207604) 6306399 ns/iter (± 210024) 2.14
iffts/simd ifft/25 132322847 ns/iter (± 1934509) 65891527 ns/iter (± 1735211) 2.01
merkle throughput/simd merkle 29899871 ns/iter (± 689114) 13712527 ns/iter (± 579195) 2.18

This comment was automatically generated by workflow using github-action-benchmark.

CC: @shaharsamocha7

Copy link
Collaborator

@ohad-starkware ohad-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: with comment

Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status: 1 of 5 files reviewed, all discussions resolved


crates/prover/src/constraint_framework/expr.rs line 403 at r1 (raw file):

    fn add_constraint<G>(&mut self, constraint: G)
    where
        Self::EF: std::ops::Mul<G, Output = Self::EF> + From<G>,

non blocking

Suggestion:

 Self::EF: From<G

@Alon-Ti Alon-Ti force-pushed the alont/simplify-expr branch from 8a20339 to 1d9f08c Compare November 25, 2024 15:31
@Alon-Ti Alon-Ti force-pushed the alont/remove-evaluator-mul branch 2 times, most recently from d79419f to 96dfdd1 Compare November 26, 2024 08:25
@Alon-Ti Alon-Ti force-pushed the alont/simplify-expr branch 2 times, most recently from b5e6acc to 188ba5c Compare November 26, 2024 08:28
@Alon-Ti Alon-Ti force-pushed the alont/remove-evaluator-mul branch from 96dfdd1 to eb99416 Compare November 26, 2024 08:28
@Alon-Ti Alon-Ti force-pushed the alont/simplify-expr branch from 188ba5c to e4c2d1d Compare November 26, 2024 13:49
@Alon-Ti Alon-Ti force-pushed the alont/remove-evaluator-mul branch from eb99416 to 9523340 Compare November 26, 2024 14:43
@Alon-Ti Alon-Ti force-pushed the alont/simplify-expr branch from e4c2d1d to d53fec3 Compare November 27, 2024 11:17
@Alon-Ti Alon-Ti force-pushed the alont/remove-evaluator-mul branch from 9523340 to 7320a5d Compare November 27, 2024 11:17
Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 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 3 of 5 files at r1, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Alon-Ti)

@Alon-Ti Alon-Ti changed the base branch from alont/simplify-expr to graphite-base/901 November 28, 2024 15:24
@Alon-Ti Alon-Ti force-pushed the alont/remove-evaluator-mul branch from 7320a5d to e48db0a Compare November 28, 2024 15:24
@Alon-Ti Alon-Ti changed the base branch from graphite-base/901 to dev November 28, 2024 15:25
@Alon-Ti Alon-Ti force-pushed the alont/remove-evaluator-mul branch from e48db0a to 72b28de Compare November 28, 2024 15:25
@Alon-Ti Alon-Ti merged commit e0d930e into dev Nov 28, 2024
16 of 19 checks passed
Copy link
Contributor Author

Alon-Ti commented Nov 28, 2024

Merge activity

  • Nov 28, 10:29 AM EST: A user merged this pull request with Graphite.

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.

5 participants