fix 1q matrix bug in Quantum Shannon Decomposer #10126

ewinston · 2023-05-17T15:30:11Z

Summary

Fixes #10036

Details and comments

qiskit-bot · 2023-05-17T15:30:17Z

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@ikkoham

coveralls · 2023-05-17T16:25:22Z

Pull Request Test Coverage Report for Build 5202288191

3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
67 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.06%) to 85.907%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/quantum_info/synthesis/qsd.py	3	4	75.0%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/sabre_swap/mod.rs	1	99.77%
crates/qasm2/src/expr.rs	1	93.76%
qiskit/circuit/tools/pi_check.py	1	91.23%
crates/qasm2/src/lex.rs	3	90.89%
qiskit/pulse/library/waveform.py	3	93.75%
qiskit/pulse/macros.py	3	94.67%
qiskit/pulse/filters.py	6	94.12%
crates/qasm2/src/parse.rs	12	97.11%
qiskit/visualization/circuit/circuit_visualization.py	13	69.91%
qiskit/pulse/schedule.py	24	88.27%

Totals
Change from base Build 5177933484:	0.06%
Covered Lines:	71257
Relevant Lines:	82947

💛 - Coveralls

jlapeyre · 2023-05-17T20:08:19Z

I think this example from the bug report #10036 still fails with this commit

qs_decomposition(qiskit.quantum_info.random_unitary(4).to_matrix())

Not only the 1q example fails, but some two qubit unitaries also result in an error.

I think the fix given in the bug report looks like a correct fix: for some 2q input matrices ind2q is empty so that mat2 is not bound.

jlapeyre · 2023-05-18T15:39:23Z

I added a short fix and a test for a matrix that still failed with this PR, and made a PR to this PR branch ewinston#4

The matrix I used in the test is CXGate().to_matrix(). Note that the qsd decomposition is more complicated than the input, but at least it does not throw an error.

ewinston · 2023-05-23T14:40:19Z

@jlapeyre Since the fix you and @jsmallz333 proposed works regardless of my approach maybe you should just submit that pr separately and I'll close this one.

jlapeyre · 2023-05-30T16:45:20Z

@ewinston , as we discussed out-of-band I think that it makes sense to include the commits you made in this PR as well. This will avoid calling the synthesis method in some cases where it can't do any good.

Unless I misunderstand, I think you can just commit ewinston#4 in order to include the commits that fix the bug proper. If you prefer separating the PRs, I'm fine with making a separate PR as you suggest above.

Co-authored-by: jsmallz333 <90203920+jsmallz333@users.noreply.github.com>

Add check that decomposition includes qsd2q gates before optimizing them

jlapeyre · 2023-06-05T14:51:50Z

This looks good to go. But I think someone other than me should merge, since I added a commit to this PR.

mtreinish

This LGTM, I just have a small nit inline about the comments. ~~I'm going to apply the suggestion and enqueue for merging.~~ (I forgot about the missing release note).

test/python/quantum_info/test_synthesis.py

mtreinish

Oh, I also forgot about a release note. Would you mine adding one so we can document we fixed this in the next bugfix release.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

jlapeyre · 2023-06-05T23:09:38Z

I also forgot about a release note.

Yes, of course. I forgot too. This is done in a PR to this PR branch ewinston#5

ewinston · 2023-06-06T15:09:40Z

Thanks @jlapeyre

mtreinish

This LGTM, but I think it's still missing a release note to cover the bug fix

jlapeyre · 2023-07-10T15:51:47Z

I pushed a new commit to
Add release note for fix to issue 10036 #5
to make a requested change in the release note.

This was requested in a review comment.

Add release note for fix to issue 10036

mtreinish

LGTM, thanks for adding the release note.

* fix 1q bug * formatting * restrict 2q gates from apply_a2 * Add check that decomposition includes qsd2q gates before optimizing them Co-authored-by: jsmallz333 <90203920+jsmallz333@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Add release note for fix to issue 10036 * avoid creating unitary gate if initial gate is 2q * remove debug code * Change "certain" to "trivial" This was requested in a review comment. --------- Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> Co-authored-by: jsmallz333 <90203920+jsmallz333@users.noreply.github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> (cherry picked from commit b831bcf)

* fix 1q bug * formatting * restrict 2q gates from apply_a2 * Add check that decomposition includes qsd2q gates before optimizing them Co-authored-by: jsmallz333 <90203920+jsmallz333@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Add release note for fix to issue 10036 * avoid creating unitary gate if initial gate is 2q * remove debug code * Change "certain" to "trivial" This was requested in a review comment. --------- Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> Co-authored-by: jsmallz333 <90203920+jsmallz333@users.noreply.github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> (cherry picked from commit b831bcf) Co-authored-by: ewinston <ewinston@us.ibm.com>

fix 1q bug

370d3fd

ewinston requested review from a team, alexanderivrii, ShellyGarion and ikkoham as code owners May 17, 2023 15:30

formatting

16f1536

mtreinish added the Changelog: Bugfix Include in the "Fixed" section of the changelog label May 17, 2023

mtreinish added this to the 0.24.1 milestone May 17, 2023

mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label May 17, 2023

jlapeyre mentioned this pull request May 18, 2023

Add check that decomposition includes qsd2q gates before optimizing them ewinston/qiskit#4

Merged

restrict 2q gates from apply_a2

d5bec1b

Merge branch 'main' into issue/10036

38beb2d

jlapeyre mentioned this pull request May 31, 2023

Prepare 0.24.1 #10187

Merged

4 tasks

jlapeyre added 2 commits May 31, 2023 22:55

Merge branch 'main' into issue/10036

300c8ab

Merge branch 'main' into issue/10036

3ee18a1

jlapeyre mentioned this pull request Jun 1, 2023

Quantum shannon decomposition failing for some inputs #10036

Closed

jlapeyre and others added 2 commits June 1, 2023 13:02

Add check that decomposition includes qsd2q gates before optimizing them

d40a28b

Co-authored-by: jsmallz333 <90203920+jsmallz333@users.noreply.github.com>

Merge branch 'issue/10036' into issue/10036-gjl

9b1b103

jlapeyre modified the milestones: 0.24.1, 0.24.2 Jun 1, 2023

ewinston and others added 2 commits June 2, 2023 12:17

Merge pull request #4 from jlapeyre/issue/10036-gjl

93733c8

Add check that decomposition includes qsd2q gates before optimizing them

Merge branch 'main' into issue/10036

7cc6d59

mtreinish reviewed Jun 5, 2023

View reviewed changes

test/python/quantum_info/test_synthesis.py Outdated Show resolved Hide resolved

test/python/quantum_info/test_synthesis.py Outdated Show resolved Hide resolved

mtreinish reviewed Jun 5, 2023

View reviewed changes

jlapeyre and others added 2 commits June 5, 2023 18:26

Apply suggestions from code review

93ec8d8

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Add release note for fix to issue 10036

5171008

ewinston added 2 commits June 7, 2023 01:26

avoid creating unitary gate if initial gate is 2q

b8748d6

remove debug code

2a59689

mtreinish reviewed Jul 10, 2023

View reviewed changes

jlapeyre and others added 3 commits July 10, 2023 12:51

Change "certain" to "trivial"

810759d

This was requested in a review comment.

Merge branch 'main' into issue/10036

37f9165

Merge pull request #5 from jlapeyre/issue/10036-releasenote

0a9c7a1

Add release note for fix to issue 10036

mtreinish approved these changes Jul 18, 2023

View reviewed changes

mtreinish added this pull request to the merge queue Jul 18, 2023

Merged via the queue into Qiskit:main with commit b831bcf Jul 18, 2023

mergify bot mentioned this pull request Jul 18, 2023

fix 1q matrix bug in Quantum Shannon Decomposer (backport #10126) #10446

Merged

jakelishman mentioned this pull request Sep 6, 2023

Gate.control method throwing error #10787

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix 1q matrix bug in Quantum Shannon Decomposer #10126

fix 1q matrix bug in Quantum Shannon Decomposer #10126

ewinston commented May 17, 2023 •

edited by kdk

Loading

qiskit-bot commented May 17, 2023

coveralls commented May 17, 2023 •

edited

Loading

jlapeyre commented May 17, 2023 •

edited

Loading

jlapeyre commented May 18, 2023 •

edited

Loading

ewinston commented May 23, 2023

jlapeyre commented May 30, 2023

jlapeyre commented Jun 5, 2023

mtreinish left a comment •

edited

Loading

mtreinish left a comment

jlapeyre commented Jun 5, 2023

ewinston commented Jun 6, 2023

mtreinish left a comment

jlapeyre commented Jul 10, 2023 •

edited

Loading

mtreinish left a comment

fix 1q matrix bug in Quantum Shannon Decomposer #10126

fix 1q matrix bug in Quantum Shannon Decomposer #10126

Conversation

ewinston commented May 17, 2023 • edited by kdk Loading

Summary

Details and comments

qiskit-bot commented May 17, 2023

coveralls commented May 17, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5202288191

💛 - Coveralls

jlapeyre commented May 17, 2023 • edited Loading

jlapeyre commented May 18, 2023 • edited Loading

ewinston commented May 23, 2023

jlapeyre commented May 30, 2023

jlapeyre commented Jun 5, 2023

mtreinish left a comment • edited Loading

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

jlapeyre commented Jun 5, 2023

ewinston commented Jun 6, 2023

mtreinish left a comment

Choose a reason for hiding this comment

jlapeyre commented Jul 10, 2023 • edited Loading

mtreinish left a comment

Choose a reason for hiding this comment

ewinston commented May 17, 2023 •

edited by kdk

Loading

coveralls commented May 17, 2023 •

edited

Loading

jlapeyre commented May 17, 2023 •

edited

Loading

jlapeyre commented May 18, 2023 •

edited

Loading

mtreinish left a comment •

edited

Loading

jlapeyre commented Jul 10, 2023 •

edited

Loading