Oxidize TwoQubitDecomposeUpToDiagonal #12563

mtreinish · 2024-06-13T08:50:01Z

Summary

This commit ports the TwoQubitDecomposeUpToDiagonal class from Python to rust. This internal private class is used internally by the quantum shannon decomposition code, and while not performance critical was simple to port. One difference is while the original Python implementation was a class, it acted more like a function in practice. So the new rust version is exposed as a function.

Details and comments

TODO:

Generate circuit rust side after Add infrastructure for gates, instruction, and operations in Rust #12459 merges

This commit ports the TwoQubitDecomposeUpToDiagonal class from Python to rust. This internal private class is used internally by the quantum shannon decomposition code, and while not performance critical was simple to port. One difference is while the original Python implementation was a class, it acted more like a function in practice. So the new rust version is exposed as a function. Co-authored-by: Luciano Bello <bel@zurich.ibm.com> Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> Co-authored-by: Sebastian Brandhofer <148463728+sbrandhsn@users.noreply.github.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> Co-authored-by: Julien Gacon <gaconju@gmail.com> Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com> Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Co-authored-by: Henry Zou <87874865+henryzou50@users.noreply.github.com> Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>

coveralls · 2024-06-13T09:17:58Z

Pull Request Test Coverage Report for Build 9496622203

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

213 of 224 (95.09%) changed or added relevant lines in 2 files are covered.
18 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.01%) to 89.593%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/two_qubit_decompose.rs	207	218	94.95%

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

Totals
Change from base Build 9483656494:	-0.01%
Covered Lines:	62661
Relevant Lines:	69940

💛 - Coveralls

Since Qiskit#12459 recently merged we now have a mechanism to build a circuit from rust. This commit updates the synthesis function to build the circuit directly in rust instead of returning a circuit sequence and building the circuit from Python. This should speed up the construction substantially.

qiskit-bot · 2024-06-14T05:36:49Z

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

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

This commit removes the Python implementation of the function. This is now unused in Qiskit and was never a public class so nothing external should be depending on it. Since it's not used we should just remove it.

coveralls · 2024-06-14T07:30:23Z

Pull Request Test Coverage Report for Build 9512074968

Details

228 of 239 (95.4%) changed or added relevant lines in 2 files are covered.
9 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.04%) to 89.749%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/two_qubit_decompose.rs	224	235	95.32%

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

Totals
Change from base Build 9503795487:	0.04%
Covered Lines:	63527
Relevant Lines:	70783

💛 - Coveralls

coveralls · 2024-06-26T13:08:20Z

Pull Request Test Coverage Report for Build 9679860893

Details

228 of 239 (95.4%) changed or added relevant lines in 2 files are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 89.782%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/two_qubit_decompose.rs	224	235	95.32%

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

Totals
Change from base Build 9676629284:	0.02%
Covered Lines:	63914
Relevant Lines:	71188

💛 - Coveralls

coveralls · 2024-06-26T14:43:56Z

Pull Request Test Coverage Report for Build 9681374865

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

228 of 239 (95.4%) changed or added relevant lines in 2 files are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.004%) to 89.764%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/two_qubit_decompose.rs	224	235	95.32%

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

Totals
Change from base Build 9676629284:	0.004%
Covered Lines:	63901
Relevant Lines:	71188

💛 - Coveralls

coveralls · 2024-07-24T12:00:05Z

Pull Request Test Coverage Report for Build 10080593333

Details

226 of 237 (95.36%) changed or added relevant lines in 2 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.03%) to 89.922%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/two_qubit_decompose.rs	222	233	95.28%

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

Totals
Change from base Build 10075234532:	0.03%
Covered Lines:	65974
Relevant Lines:	73368

💛 - Coveralls

kevinhartman

Looks good, just a few super minor comments.

kevinhartman · 2024-07-24T15:07:58Z

crates/accelerate/src/two_qubit_decompose.rs

+
+    #[new]
+    #[pyo3(signature=(unitary_matrix, fidelity=DEFAULT_FIDELITY, _specialization=None))]
+    fn new(


Is there value in following the pattern of naming this py_new and naming the Rust-side constructor new?

The pattern that's used everywhere in accelerate for this was rust is foo_inner and python is foo. I think we should normalize on a single pattern across all the crates, but maybe do that in a separate standalone PR?

crates/accelerate/src/two_qubit_decompose.rs

* Oxidize TwoQubitDecomposeUpToDiagonal This commit ports the TwoQubitDecomposeUpToDiagonal class from Python to rust. This internal private class is used internally by the quantum shannon decomposition code, and while not performance critical was simple to port. One difference is while the original Python implementation was a class, it acted more like a function in practice. So the new rust version is exposed as a function. Co-authored-by: Luciano Bello <bel@zurich.ibm.com> Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> Co-authored-by: Sebastian Brandhofer <148463728+sbrandhsn@users.noreply.github.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> Co-authored-by: Julien Gacon <gaconju@gmail.com> Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com> Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Co-authored-by: Henry Zou <87874865+henryzou50@users.noreply.github.com> Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> * Build circuit from rust Since Qiskit#12459 recently merged we now have a mechanism to build a circuit from rust. This commit updates the synthesis function to build the circuit directly in rust instead of returning a circuit sequence and building the circuit from Python. This should speed up the construction substantially. * Remove unused private Python class This commit removes the Python implementation of the function. This is now unused in Qiskit and was never a public class so nothing external should be depending on it. Since it's not used we should just remove it. * Remove unused import * Calculate best_nbasis in unwrap_or_else() --------- Co-authored-by: Luciano Bello <bel@zurich.ibm.com> Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> Co-authored-by: Sebastian Brandhofer <148463728+sbrandhsn@users.noreply.github.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> Co-authored-by: Julien Gacon <gaconju@gmail.com> Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com> Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Co-authored-by: Henry Zou <87874865+henryzou50@users.noreply.github.com> Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>

mtreinish added Changelog: None Do not include in changelog synthesis Rust This PR or issue is related to Rust code in the repository labels Jun 13, 2024

mtreinish added 2 commits June 14, 2024 01:08

Merge remote-tracking branch 'origin/main' into oxidize-up-to-diagonal

9fc6d2d

mtreinish marked this pull request as ready for review June 14, 2024 05:36

mtreinish requested review from alexanderivrii, ShellyGarion and a team as code owners June 14, 2024 05:36

Remove unused private Python class

c56f8af

This commit removes the Python implementation of the function. This is now unused in Qiskit and was never a public class so nothing external should be depending on it. Since it's not used we should just remove it.

mtreinish force-pushed the oxidize-up-to-diagonal branch from 8bf99c1 to c56f8af Compare June 14, 2024 07:05

Merge branch 'main' into oxidize-up-to-diagonal

e2ff812

Remove unused import

cf422e0

Merge remote-tracking branch 'origin/main' into oxidize-up-to-diagonal

ca92d7b

kevinhartman reviewed Jul 24, 2024

View reviewed changes

mtreinish added 2 commits July 24, 2024 12:38

Calculate best_nbasis in unwrap_or_else()

655ce1d

Merge branch 'main' into oxidize-up-to-diagonal

971e061

mtreinish assigned kevinhartman Jul 24, 2024

mtreinish added this to the 1.2.0 milestone Jul 24, 2024

mtreinish requested a review from kevinhartman July 24, 2024 17:19

kevinhartman approved these changes Jul 24, 2024

View reviewed changes

kevinhartman enabled auto-merge July 24, 2024 17:22

kevinhartman added this pull request to the merge queue Jul 24, 2024

Merged via the queue into Qiskit:main with commit 6a78c63 Jul 24, 2024
15 checks passed

mtreinish deleted the oxidize-up-to-diagonal branch July 24, 2024 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oxidize TwoQubitDecomposeUpToDiagonal #12563

Oxidize TwoQubitDecomposeUpToDiagonal #12563

mtreinish commented Jun 13, 2024 •

edited

Loading

coveralls commented Jun 13, 2024 •

edited

Loading

qiskit-bot commented Jun 14, 2024

coveralls commented Jun 14, 2024 •

edited

Loading

coveralls commented Jun 26, 2024 •

edited

Loading

coveralls commented Jun 26, 2024 •

edited

Loading

coveralls commented Jul 24, 2024 •

edited

Loading

kevinhartman left a comment

kevinhartman Jul 24, 2024

mtreinish Jul 24, 2024

Oxidize TwoQubitDecomposeUpToDiagonal #12563

Oxidize TwoQubitDecomposeUpToDiagonal #12563

Conversation

mtreinish commented Jun 13, 2024 • edited Loading

Summary

Details and comments

coveralls commented Jun 13, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9496622203

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

qiskit-bot commented Jun 14, 2024

coveralls commented Jun 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9512074968

Details

💛 - Coveralls

coveralls commented Jun 26, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9679860893

Details

💛 - Coveralls

coveralls commented Jun 26, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9681374865

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

coveralls commented Jul 24, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10080593333

Details

💛 - Coveralls

kevinhartman left a comment

Choose a reason for hiding this comment

kevinhartman Jul 24, 2024

Choose a reason for hiding this comment

mtreinish Jul 24, 2024

Choose a reason for hiding this comment

mtreinish commented Jun 13, 2024 •

edited

Loading

coveralls commented Jun 13, 2024 •

edited

Loading

coveralls commented Jun 14, 2024 •

edited

Loading

coveralls commented Jun 26, 2024 •

edited

Loading

coveralls commented Jun 26, 2024 •

edited

Loading

coveralls commented Jul 24, 2024 •

edited

Loading