Oxidize `synth_cnot_count_full_pmh` #12588

Cryoris · 2024-06-17T12:07:30Z

Summary

In a benchmark to synthesize 100 qubit linear networks 1000 times, the runtime is ~44 times faster:

(main) Took 172.75505709648132s
(this PR) Took 3.920625925064087s

I would assume the main speedup is due to a more efficient circuit construction.

Details and comments

Does not address The synthesis function synth_cnot_count_full_pmh returns invalid circuits for certain section_size values #12106, which is currently tackled in a different PR.
There certainly is some way to use iterators more by iterating over sections, but I couldn't yet wrap my head around it (and it might not give a significant speedup), suggestions are welcome 🙂

qiskit-bot · 2024-06-17T12:07:35Z

One or more of the following people are relevant to this code:

@Eric-Arellano
@Qiskit/terra-core
@kevinhartman
@mtreinish

coveralls · 2024-06-17T12:29:37Z

Pull Request Test Coverage Report for Build 9547358808

Details

115 of 115 (100.0%) changed or added relevant lines in 5 files are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.05%) to 89.764%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	6	92.11%

Totals
Change from base Build 9514566193:	0.05%
Covered Lines:	63489
Relevant Lines:	70729

💛 - Coveralls

coveralls · 2024-06-17T14:05:30Z

Pull Request Test Coverage Report for Build 9548795140

Details

115 of 115 (100.0%) changed or added relevant lines in 5 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.06%) to 89.769%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	93.38%

Totals
Change from base Build 9514566193:	0.06%
Covered Lines:	63491
Relevant Lines:	70727

💛 - Coveralls

ShellyGarion

Generally, it looks very well. Please see some comments and questions.
Also, release notes are needed.

crates/accelerate/src/synthesis/cnot_synthesis.rs

crates/accelerate/src/lib.rs

crates/accelerate/src/synthesis/cnot_synthesis.rs

coveralls · 2024-06-20T07:18:03Z

Pull Request Test Coverage Report for Build 9592962940

Details

113 of 113 (100.0%) changed or added relevant lines in 5 files are covered.
11 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 89.776%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	5	92.88%
crates/qasm2/src/parse.rs	6	97.61%

Totals
Change from base Build 9585515412:	0.02%
Covered Lines:	63645
Relevant Lines:	70893

💛 - Coveralls

crates/accelerate/src/synthesis/cnot_synthesis.rs

coveralls · 2024-06-20T16:23:24Z

Pull Request Test Coverage Report for Build 9600662838

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

116 of 116 (100.0%) changed or added relevant lines in 5 files are covered.
45 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.03%) to 89.724%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	5	92.11%
crates/qasm2/src/parse.rs	12	96.23%
qiskit/visualization/pulse_v2/device_info.py	28	39.71%

Totals
Change from base Build 9585515412:	-0.03%
Covered Lines:	63627
Relevant Lines:	70914

💛 - Coveralls

coveralls · 2024-06-21T07:20:54Z

Pull Request Test Coverage Report for Build 9609475087

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

113 of 113 (100.0%) changed or added relevant lines in 5 files are covered.
54 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.006%) to 89.762%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/lex.rs	2	93.13%
qiskit/circuit/quantumcircuit.py	23	95.69%
qiskit/visualization/pulse_v2/device_info.py	28	39.71%

Totals
Change from base Build 9585515412:	0.006%
Covered Lines:	63652
Relevant Lines:	70912

💛 - Coveralls

ShellyGarion

This code looks good, but it should be refactored as #12456 was merged.

My main comment is that perhaps we should consider incorporating the suggestion in
#12166 (comment) to handle invalid values section_size as well as tests to verify the correctness of the valid circuit (see #12166 (comment)).
Then, perhaps it would be better to have the default section_size as log(n) since it produces a circuit with smaller CX count.

crates/accelerate/src/synthesis/cnot_synthesis.rs

qiskit/synthesis/linear/cnot_synth.py

releasenotes/notes/oxidize-pmh-ec74e4002510eaad.yaml

Co-authored-by: Abdalla01001 <[email protected]> Co-authored-by: Tarun-Kumar07 <[email protected]>

coveralls · 2024-07-03T16:38:43Z

Pull Request Test Coverage Report for Build 9781347471

Details

113 of 113 (100.0%) changed or added relevant lines in 3 files are covered.
12 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.003%) to 89.843%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/lex.rs	5	91.86%
crates/qasm2/src/parse.rs	6	97.61%

Totals
Change from base Build 9780558630:	0.003%
Covered Lines:	65198
Relevant Lines:	72569

💛 - Coveralls

ShellyGarion

Thanks @Cryoris - this PR looks great, I have some small comments.

qiskit/synthesis/linear/cnot_synth.py

releasenotes/notes/oxidize-pmh-ec74e4002510eaad.yaml

test/python/synthesis/test_linear_synthesis.py

alexanderivrii

The code looks amazing! I just had one tiny question about function arguments.

crates/accelerate/src/synthesis/linear/pmh.rs

coveralls · 2024-07-04T09:00:34Z

Pull Request Test Coverage Report for Build 9791122293

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

113 of 113 (100.0%) changed or added relevant lines in 3 files are covered.
12 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.01%) to 89.85%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.11%
crates/circuit/src/circuit_data.rs	9	90.73%

Totals
Change from base Build 9780558630:	0.01%
Covered Lines:	65199
Relevant Lines:	72564

💛 - Coveralls

ShellyGarion

LGTM! Thanks @Cryoris !

* port PMH synth to Rust * Raise an error if the section size is invalid Co-authored-by: Abdalla01001 <[email protected]> Co-authored-by: Tarun-Kumar07 <[email protected]> * Review comments of Shelly & Sasha --------- Co-authored-by: Abdalla01001 <[email protected]> Co-authored-by: Tarun-Kumar07 <[email protected]>

Cryoris added performance Rust This PR or issue is related to Rust code in the repository labels Jun 17, 2024

Cryoris requested review from alexanderivrii, ShellyGarion and a team as code owners June 17, 2024 12:07

ShellyGarion reviewed Jun 18, 2024

View reviewed changes

crates/accelerate/src/synthesis/cnot_synthesis.rs Outdated Show resolved Hide resolved

ShellyGarion added the synthesis label Jun 18, 2024

ShellyGarion added this to the 1.2.0 milestone Jun 20, 2024

ShellyGarion reviewed Jun 20, 2024

View reviewed changes

crates/accelerate/src/synthesis/cnot_synthesis.rs Outdated Show resolved Hide resolved

ShellyGarion requested changes Jun 30, 2024

View reviewed changes

ShellyGarion reviewed Jun 30, 2024

View reviewed changes

qiskit/synthesis/linear/cnot_synth.py Outdated Show resolved Hide resolved

alexanderivrii mentioned this pull request Jul 3, 2024

fixing synthesis release notes #12715

Merged

alexanderivrii reviewed Jul 3, 2024

View reviewed changes

releasenotes/notes/oxidize-pmh-ec74e4002510eaad.yaml Outdated Show resolved Hide resolved

Cryoris requested review from nonhermitian, eggerdj, wshanks and jyu00 as code owners July 3, 2024 11:16

ShellyGarion mentioned this pull request Jul 3, 2024

virtual padding for invalid section_size in synth_cnot_count_full_pmh #12712

Closed

ShellyGarion linked an issue Jul 3, 2024 that may be closed by this pull request

The synthesis function synth_cnot_count_full_pmh returns invalid circuits for certain section_size values #12106

Closed

Cryoris and others added 2 commits July 3, 2024 18:09

port PMH synth to Rust

a0a9325

Raise an error if the section size is invalid

97181ed

Co-authored-by: Abdalla01001 <[email protected]> Co-authored-by: Tarun-Kumar07 <[email protected]>

Cryoris force-pushed the rust/pmh branch from 42a0e27 to 97181ed Compare July 3, 2024 16:13

ShellyGarion removed the request for review from wshanks July 3, 2024 16:14

ShellyGarion removed request for nonhermitian, jyu00 and eggerdj July 3, 2024 16:14

ShellyGarion reviewed Jul 4, 2024

View reviewed changes

alexanderivrii reviewed Jul 4, 2024

View reviewed changes

crates/accelerate/src/synthesis/linear/pmh.rs Outdated Show resolved Hide resolved

crates/accelerate/src/synthesis/linear/pmh.rs Outdated Show resolved Hide resolved

Review comments of Shelly & Sasha

ca65dec

ShellyGarion approved these changes Jul 4, 2024

View reviewed changes

Cryoris added this pull request to the merge queue Jul 4, 2024

Merged via the queue into Qiskit:main with commit 722a5bc Jul 4, 2024
15 checks passed

Cryoris deleted the rust/pmh branch July 10, 2024 09:50

ElePT added Changelog: Bugfix Include in the "Fixed" section of the changelog Changelog: New Feature Include in the "Added" section of the changelog labels Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oxidize `synth_cnot_count_full_pmh` #12588

Oxidize `synth_cnot_count_full_pmh` #12588

Cryoris commented Jun 17, 2024

qiskit-bot commented Jun 17, 2024

coveralls commented Jun 17, 2024 •

edited

Loading

coveralls commented Jun 17, 2024 •

edited

Loading

ShellyGarion left a comment

coveralls commented Jun 20, 2024 •

edited

Loading

coveralls commented Jun 20, 2024 •

edited

Loading

coveralls commented Jun 21, 2024 •

edited

Loading

ShellyGarion left a comment

coveralls commented Jul 3, 2024 •

edited

Loading

ShellyGarion left a comment

alexanderivrii left a comment

coveralls commented Jul 4, 2024 •

edited

Loading

ShellyGarion left a comment

Oxidize synth_cnot_count_full_pmh #12588

Oxidize synth_cnot_count_full_pmh #12588

Conversation

Cryoris commented Jun 17, 2024

Summary

Details and comments

qiskit-bot commented Jun 17, 2024

coveralls commented Jun 17, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9547358808

Details

💛 - Coveralls

coveralls commented Jun 17, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9548795140

Details

💛 - Coveralls

ShellyGarion left a comment

Choose a reason for hiding this comment

coveralls commented Jun 20, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9592962940

Details

💛 - Coveralls

coveralls commented Jun 20, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9600662838

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

coveralls commented Jun 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9609475087

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ShellyGarion left a comment

Choose a reason for hiding this comment

coveralls commented Jul 3, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9781347471

Details

💛 - Coveralls

ShellyGarion left a comment

Choose a reason for hiding this comment

alexanderivrii left a comment

Choose a reason for hiding this comment

coveralls commented Jul 4, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9791122293

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ShellyGarion left a comment

Choose a reason for hiding this comment

Oxidize `synth_cnot_count_full_pmh` #12588

Oxidize `synth_cnot_count_full_pmh` #12588

coveralls commented Jun 17, 2024 •

edited

Loading

coveralls commented Jun 17, 2024 •

edited

Loading

coveralls commented Jun 20, 2024 •

edited

Loading

coveralls commented Jun 20, 2024 •

edited

Loading

coveralls commented Jun 21, 2024 •

edited

Loading

coveralls commented Jul 3, 2024 •

edited

Loading

coveralls commented Jul 4, 2024 •

edited

Loading