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.

Sign up for GitHub

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

Jump to bottom

feat!: Use ConvexChecker in CircuitMatch. #77

Merged
merged 2 commits into from
Sep 4, 2023
Merged

feat!: Use ConvexChecker in CircuitMatch. #77

merged 2 commits into from
Sep 4, 2023

Conversation

lmondada
Copy link
Contributor

@lmondada lmondada commented Sep 4, 2023

BREAKING CHANGE: CircuitMatcher::find_rooted_matches is no longer public.

@lmondada lmondada requested a review from aborgna-q September 4, 2023 08:44
@lmondada
feat!: Use ConvexChecker in CircuitMatch.
7968766 
BREAKING CHANGE: CircuitMatcher::find_rooted_matches is no longer public.
@lmondada lmondada force-pushed the feat/convex-checker branch from 31ae721 to 7968766 Compare September 4, 2023 08:47
@lmondada
Copy link
Contributor Author

lmondada commented Sep 4, 2023

Benchmarks have improved by 50-75%...

     Running benches/bench_main.rs (target/release/deps/bench_main-40c6955087839f08)
Gnuplot not found, using plotters backend
Loading patterns...
Loaded 4913 patterns
Loaded circuit
Building matcher for n = 200...
Pattern matching/TKET2/200
                        time:   [5.4931 ms 5.6201 ms 5.7792 ms]
                        change: [-60.225% -59.010% -57.657%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high mild
Building matcher for n = 400...
Pattern matching/TKET2/400
                        time:   [6.8224 ms 6.8758 ms 6.9314 ms]
                        change: [-59.805% -59.108% -58.413%] (p = 0.00 < 0.05)
                        Performance has improved.
Building matcher for n = 600...
Pattern matching/TKET2/600
                        time:   [7.4682 ms 7.5225 ms 7.5753 ms]
                        change: [-58.407% -57.649% -56.808%] (p = 0.00 < 0.05)
                        Performance has improved.
Building matcher for n = 800...
Pattern matching/TKET2/800
                        time:   [7.6114 ms 7.6673 ms 7.7328 ms]
                        change: [-55.799% -55.107% -54.400%] (p = 0.00 < 0.05)
                        Performance has improved.
Building matcher for n = 1000...
Pattern matching/TKET2/1000
                        time:   [17.702 ms 17.913 ms 18.136 ms]
                        change: [-75.470% -74.884% -74.262%] (p = 0.00 < 0.05)
                        Performance has improved.
Building matcher for n = 1200...
Pattern matching/TKET2/1200
                        time:   [20.031 ms 20.338 ms 20.638 ms]
                        change: [-72.707% -72.161% -71.559%] (p = 0.00 < 0.05)
                        Performance has improved.
Building matcher for n = 1400...
Pattern matching/TKET2/1400
                        time:   [21.853 ms 22.195 ms 22.624 ms]
                        change: [-71.205% -70.578% -69.881%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 10 measurements (20.00%)
  2 (20.00%) high severe
Building matcher for n = 1600...
Pattern matching/TKET2/1600
                        time:   [23.130 ms 23.458 ms 23.781 ms]
                        change: [-74.121% -72.779% -71.412%] (p = 0.00 < 0.05)
                        Performance has improved.
Building matcher for n = 1800...
Pattern matching/TKET2/1800
                        time:   [24.532 ms 24.768 ms 25.009 ms]
                        change: [-71.328% -70.681% -70.043%] (p = 0.00 < 0.05)
                        Performance has improved.
Building matcher for n = 2000...
Pattern matching/TKET2/2000
                        time:   [25.423 ms 25.795 ms 26.257 ms]
                        change: [-77.616% -75.239% -72.542%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 10 measurements (10.00%)

aborgna-q
aborgna-q approved these changes Sep 4, 2023
View reviewed changes
src/portmatching/matcher.rs
.collect()
}

/// Find all convex pattern matches in a circuit rooted at a given node.
pub fn find_rooted_matches<'a, 'm, C: Circuit<'a>>(
fn find_rooted_matches<'a, 'm, C: Circuit<'a>>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍
Was there any reason for the rooted match to pub before?
Would a user care about matching on an specific root?

Copy link
Contributor Author

@lmondada lmondada Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There is currently no way for the user to choose the root of a pattern themselves, so being able to find rooted matches is pretty pointless. In hindsight it should never have been public.

@lmondada lmondada merged commit 5343671 into main Sep 4, 2023
7 checks passed
@lmondada lmondada deleted the feat/convex-checker branch September 4, 2023 10:33
ss2165 pushed a commit that referenced this pull request Sep 4, 2023
@lmondada @ss2165
feat!: Use ConvexChecker in CircuitMatch. (#77)
d457bd0 
BREAKING CHANGE: CircuitMatcher::find_rooted_matches is no longer
public.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aborgna-q aborgna-q aborgna-q approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lmondada @aborgna-q