Oxidize `synth_permutation_acg` #12543

Cryoris · 2024-06-11T09:48:57Z

Summary

Closes #12226. This could in future be further improved by constructing the circuit in Rust directly.

Details and comments

A small benchmark for 1000 qubit circuit on random circuits with depth 1000 gives

(main) Took 46.50923681259155s for 10000 reps on 1000 qubits
(this PR) Took 16.09170889854431s for 10000 reps on 1000 qubits

It seems that 99% of this improvement comes from constructing the circuit in Rust, not from porting doing the permutation logic.

I'm happy about any comments or possible improvements, as this is my first Rust PR 🙂

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

maybe faster possible...

qiskit-bot · 2024-06-11T09:49:04Z

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

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

coveralls · 2024-06-11T10:10:02Z

Pull Request Test Coverage Report for Build 9463371888

Details

51 of 52 (98.08%) changed or added relevant lines in 2 files are covered.
27 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.03%) to 89.574%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/permutation.rs	50	51	98.04%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	9	92.11%
crates/qasm2/src/parse.rs	18	96.69%

Totals
Change from base Build 9455662900:	-0.03%
Covered Lines:	62525
Relevant Lines:	69803

💛 - Coveralls

ElePT

I tried taking a look as an exercise and added a few comments, I hope they can be helpful.

crates/accelerate/src/permutation.rs

coveralls · 2024-06-17T14:43:57Z

Pull Request Test Coverage Report for Build 9549394978

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

65 of 78 (83.33%) changed or added relevant lines in 2 files are covered.
18 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.04%) to 89.718%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/permutation.rs	62	75	82.67%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	6	91.35%
crates/qasm2/src/parse.rs	12	97.15%

Totals
Change from base Build 9500158747:	-0.04%
Covered Lines:	63434
Relevant Lines:	70704

💛 - Coveralls

ShellyGarion

I think that this PR also needs release notes.

coveralls · 2024-06-19T09:49:47Z

Pull Request Test Coverage Report for Build 9579566605

Details

65 of 78 (83.33%) changed or added relevant lines in 2 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.009%) to 89.76%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/permutation.rs	62	75	82.67%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.62%

Totals
Change from base Build 9578841868:	-0.009%
Covered Lines:	63612
Relevant Lines:	70869

💛 - Coveralls

Cryoris · 2024-06-21T07:53:18Z

Let's merge #12605 first to have the right directory structure.

coveralls · 2024-06-21T08:27:02Z

Pull Request Test Coverage Report for Build 9610372304

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

61 of 74 (82.43%) changed or added relevant lines in 2 files are covered.
66 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.05%) to 89.72%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/permutation.rs	58	71	81.69%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.37%
crates/qasm2/src/parse.rs	12	97.15%
qiskit/circuit/quantumcircuit.py	23	95.69%
qiskit/visualization/pulse_v2/device_info.py	28	39.71%

Totals
Change from base Build 9578841868:	-0.05%
Covered Lines:	63597
Relevant Lines:	70884

💛 - Coveralls

ShellyGarion

Generally, this code looks good. It only needs to be refactored since it conflicts with the main branch, and some further documentation is needed.

crates/accelerate/src/permutation.rs

ShellyGarion

LGTM, thanks @Cryoris !

coveralls · 2024-07-02T13:15:15Z

Pull Request Test Coverage Report for Build 9761267322

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

63 of 64 (98.44%) changed or added relevant lines in 4 files are covered.
392 unchanged lines in 9 files lost coverage.
Overall coverage decreased (-0.03%) to 89.795%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/synthesis/permutation/utils.rs	40	41	97.56%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/lex.rs	1	92.88%
crates/qasm2/src/parse.rs	12	97.15%
crates/circuit/src/circuit_data.rs	21	90.81%
crates/circuit/src/circuit_instruction.rs	31	95.09%
crates/circuit/src/dag_node.rs	32	83.67%
qiskit/dagcircuit/dagcircuit.py	56	92.15%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	58	88.35%
crates/circuit/src/operations.rs	180	78.96%

Totals
Change from base Build 9751520597:	-0.03%
Covered Lines:	64527
Relevant Lines:	71860

💛 - Coveralls

coveralls · 2024-07-02T14:00:50Z

Pull Request Test Coverage Report for Build 9762022533

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

70 of 70 (100.0%) changed or added relevant lines in 4 files are covered.
399 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.06%) to 89.77%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.11%
crates/qasm2/src/parse.rs	18	95.77%
crates/circuit/src/circuit_data.rs	21	90.81%
crates/circuit/src/circuit_instruction.rs	31	95.09%
crates/circuit/src/dag_node.rs	32	83.67%
qiskit/dagcircuit/dagcircuit.py	56	92.15%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	58	88.35%
crates/circuit/src/operations.rs	180	78.96%

Totals
Change from base Build 9751520597:	-0.06%
Covered Lines:	64514
Relevant Lines:	71866

💛 - Coveralls

After the recently merged Qiskit#12543 when working with Rust 1.79 cargo fmt makes a small formatting change that rust 1.70 wouldn't and clippy makes flags a &Vec<_> that should be a slice &[_] instead. This commit makes these two small chagnes so they're not an issue for people building with the latest stable version of rust, not just our MSRV.

After the recently merged #12543 when working with Rust 1.79 cargo fmt makes a small formatting change that rust 1.70 wouldn't and clippy makes flags a &Vec<_> that should be a slice &[_] instead. This commit makes these two small chagnes so they're not an issue for people building with the latest stable version of rust, not just our MSRV.

* Move utility functions _inverse_pattern and _get_ordered_swap to Rust * fix formatting and pylint issues * Changed input type to `PyArrayLike1<i64, AllowTypeChange>` * Refactor `permutation.rs`, clean up imports, fix coverage error * fix docstring for `_inverse_pattern` Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> * fix docstring for `_get_ordered_swap` Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> * remove pymodule nesting * remove explicit `AllowTypeChange` * Move input validation out of `_inverse_pattern` and `_get_ordered_swap` * oxidization attempt no. 1 * working version! maybe faster possible... * move more into rust & fix clones * layouting & comments * dangling comment * circuit construction in rust * remove dangling code * more lint * add reno * drop redundant Ok(expect()) * Implements Shelly's suggestions * simplify code a little --------- Co-authored-by: jpacold <jpacold@gmail.com> Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

After the recently merged Qiskit#12543 when working with Rust 1.79 cargo fmt makes a small formatting change that rust 1.70 wouldn't and clippy makes flags a &Vec<_> that should be a slice &[_] instead. This commit makes these two small chagnes so they're not an issue for people building with the latest stable version of rust, not just our MSRV.

jpacold and others added 21 commits May 2, 2024 01:41

Move utility functions _inverse_pattern and _get_ordered_swap to Rust

39bab3e

Merge branch 'Qiskit:main' into main

d278d3d

fix formatting and pylint issues

64a8e2f

Merge branch 'Qiskit:main' into main

e38b84e

Merge branch 'main' into main

7135c3a

Changed input type to PyArrayLike1<i64, AllowTypeChange>

9dc3571

Merge branch 'main' into main

71ebb9c

Refactor permutation.rs, clean up imports, fix coverage error

8b5e6b3

Merge branch 'main' into main

3fdde4f

fix docstring for _inverse_pattern

d0e347c

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

fix docstring for _get_ordered_swap

6e3bc2c

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

remove pymodule nesting

6c67959

Merge branch 'main' of github.com:jpacold/qiskit into permutation-utils

3d0b012

remove explicit AllowTypeChange

b0992b3

Move input validation out of _inverse_pattern and _get_ordered_swap

3647b16

oxidization attempt no. 1

d983a30

Merge branch 'main' of github.com:jpacold/qiskit into permutation-utils

5069588

working version!

3b7bb1c

maybe faster possible...

move more into rust & fix clones

4008814

layouting & comments

836a12c

Merge branch 'main' into permutation-utils

b4c385c

Cryoris added the Rust This PR or issue is related to Rust code in the repository label Jun 11, 2024

Cryoris requested review from alexanderivrii, ShellyGarion and a team as code owners June 11, 2024 09:48

ElePT reviewed Jun 11, 2024

View reviewed changes

crates/accelerate/src/permutation.rs Outdated Show resolved Hide resolved

crates/accelerate/src/permutation.rs Outdated Show resolved Hide resolved

crates/accelerate/src/permutation.rs Outdated Show resolved Hide resolved

crates/accelerate/src/permutation.rs Outdated Show resolved Hide resolved

ShellyGarion added the synthesis label Jun 11, 2024

dangling comment

9afeda2

ShellyGarion reviewed Jun 18, 2024

View reviewed changes

Cryoris added 2 commits June 19, 2024 11:25

add reno

2c0e33d

Merge branch 'main' into permutation-utils

f72d7d3

ShellyGarion added this to the 1.2.0 milestone Jun 20, 2024

drop redundant Ok(expect())

7cfa7f5

ShellyGarion requested changes Jun 30, 2024

View reviewed changes

crates/accelerate/src/permutation.rs Outdated Show resolved Hide resolved

crates/accelerate/src/permutation.rs Outdated Show resolved Hide resolved

crates/accelerate/src/permutation.rs Outdated Show resolved Hide resolved

Cryoris added 3 commits July 1, 2024 10:42

Merge branch 'main' into permutation-utils

2bdaa79

Implements Shelly's suggestions

0743d23

Merge branch 'main' into permutation-utils

e4b95f8

ShellyGarion previously approved these changes Jul 2, 2024

View reviewed changes

Cryoris enabled auto-merge July 2, 2024 13:33

simplify code a little

46a37aa

Cryoris dismissed ShellyGarion’s stale review via 46a37aa July 2, 2024 13:35

ShellyGarion approved these changes Jul 2, 2024

View reviewed changes

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

Merged via the queue into Qiskit:main with commit c674913 Jul 2, 2024
15 checks passed

mtreinish mentioned this pull request Jul 2, 2024

Fix cargo clippy and rustfmt for Rust 1.79 #12710

Merged

Cryoris deleted the permutation-utils branch July 10, 2024 09:50

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oxidize `synth_permutation_acg` #12543

Oxidize `synth_permutation_acg` #12543

Cryoris commented Jun 11, 2024 •

edited

Loading

qiskit-bot commented Jun 11, 2024

coveralls commented Jun 11, 2024 •

edited

Loading

ElePT left a comment

coveralls commented Jun 17, 2024 •

edited

Loading

ShellyGarion left a comment

coveralls commented Jun 19, 2024 •

edited

Loading

Cryoris commented Jun 21, 2024

coveralls commented Jun 21, 2024 •

edited

Loading

ShellyGarion left a comment

ShellyGarion left a comment

coveralls commented Jul 2, 2024 •

edited

Loading

coveralls commented Jul 2, 2024 •

edited

Loading

Oxidize synth_permutation_acg #12543

Oxidize synth_permutation_acg #12543

Conversation

Cryoris commented Jun 11, 2024 • edited Loading

Summary

Details and comments

qiskit-bot commented Jun 11, 2024

coveralls commented Jun 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9463371888

Details

💛 - Coveralls

ElePT left a comment

Choose a reason for hiding this comment

coveralls commented Jun 17, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9549394978

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ShellyGarion left a comment

Choose a reason for hiding this comment

coveralls commented Jun 19, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9579566605

Details

💛 - Coveralls

Cryoris commented Jun 21, 2024

coveralls commented Jun 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9610372304

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ShellyGarion left a comment

Choose a reason for hiding this comment

ShellyGarion left a comment

Choose a reason for hiding this comment

coveralls commented Jul 2, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9761267322

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

coveralls commented Jul 2, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9762022533

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Oxidize `synth_permutation_acg` #12543

Oxidize `synth_permutation_acg` #12543

Cryoris commented Jun 11, 2024 •

edited

Loading

coveralls commented Jun 11, 2024 •

edited

Loading

coveralls commented Jun 17, 2024 •

edited

Loading

coveralls commented Jun 19, 2024 •

edited

Loading

coveralls commented Jun 21, 2024 •

edited

Loading

coveralls commented Jul 2, 2024 •

edited

Loading

coveralls commented Jul 2, 2024 •

edited

Loading