Fix circuit drawer returning qubit[register] names for input #11096

SoranaAurelia · 2023-10-23T23:14:00Z

Summary

Circuit drawing included the full Qubit[register] label rather than just nameIndex (ex q0). This fixes #11038

Details and comments

In sabre_layout, qubit registers weren't added to the layout, resulting in the virtual qubit not being found in the registers of the layout. From here, when draw was called, the wire_label was set to qubit[register] rather then only the name and index.

qiskit-bot · 2023-10-23T23:14:05Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

@Qiskit/terra-core

coveralls · 2023-10-31T10:44:21Z

Pull Request Test Coverage Report for Build 6761670924

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

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

Files with Coverage Reduction	New Missed Lines	%
qiskit/pulse/library/waveform.py	3	93.75%
crates/qasm2/src/lex.rs	4	91.67%
qiskit/transpiler/target.py	5	93.78%
crates/qasm2/src/parse.rs	6	96.2%
qiskit/transpiler/passes/layout/vf2_post_layout.py	15	89.06%

Totals
Change from base Build 6738287566:	0.05%
Covered Lines:	74358
Relevant Lines:	85543

💛 - Coveralls

ElePT

The change looks good to me after reading the discussion in #11038. @SoranaAurelia, we would need a release note documenting the fix. Let me know if you need help with that.

ElePT

We would also benefit from a unit test to help catch any regression in the future (thx @jakelishman for pointing this out to me). We have visual unit tests that check the output of the circuit drawer. You could add a new test to test/visual/mpl/circuit/test_circuit_matplotlib_drawer.py that draws a circuit reproducing the fixed bug (this one would work) and compares it to a reference png with the correct/expected output (that you would have to save in test/visual/mpl/circuit/references).

SoranaAurelia · 2023-11-01T16:42:19Z

Okk thank you a lot for pointing it out! I'll add the changes as soon as possible!

jakelishman · 2023-11-02T16:00:44Z

test/visual/mpl/circuit/test_circuit_matplotlib_drawer.py

+        qc = QuantumCircuit(3)
+        qc.cx(0, 1)
+        qc.cx(1, 2)
+        qc.cx(2, 0)
+        circuit = transpile(qc, backend, basis_gates=["rz", "sx", "cx"], optimization_level=1)
+


You might want to pin the layout_method to Sabre here, just to be sure we're always testing it if the pipelines change in the future. We also should seed the transpiler, since there's the possibility of randomness changing the result.

…ia/qiskit into issue_11038_draw_fix

jakelishman

This looks right to me now, thanks both. Assuming it passes the test, it looks correct now.

ElePT

And thank you very much @SoranaAurelia, I ended up taking care of the reno and test myself because this is a very useful fix and we wanted to make sure it was included in today's release.

SoranaAurelia · 2023-11-02T17:53:40Z

Thanks for that! I was a bit busy at uni and work today, so thanks for taking care of it!

SoranaAurelia · 2023-11-02T18:19:33Z

The test also fails locally for me. Does this happen to you as well @ElePT ?

jakelishman · 2023-11-02T18:23:25Z

That's ok, we'll push it from the release today in the interest of timing (even if the fix is completely trivial, it'll take ~2 hours to reach the release branch from now), and squash it in a 0.45.1 patch release soon after. It's not a critical blocking bug, just annoying that we found it during the release candidate period and didn't quite have time to get it fixed before the final release.

SoranaAurelia · 2023-11-02T18:31:28Z

I think changing the image from the reference folder fixed it. The difference that was 0.98.. is now passing the assert. Should I commit it?

jakelishman · 2023-11-02T18:35:25Z

If you're able to hold off til tomorrow, it'd help out a shade with our CI throughput while we're trying to manage the release tonight? No big deal if not, though - we can just postpone its CI run til tomorrow.

SoranaAurelia · 2023-11-02T18:38:13Z

No problem! Tomorrow it is

ElePT

Thanks for your help re-generating the image, I am not sure why my environment generated the wrong pixel size. I'll re-approve :)

* added registers to layout in sabre_layout pass * Add reno and test * Fix layout method in test * Set transpiler seed * modified reference circuit for test --------- Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> (cherry picked from commit 179d9ab)

…#11199) * added registers to layout in sabre_layout pass * Add reno and test * Fix layout method in test * Set transpiler seed * modified reference circuit for test --------- Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> (cherry picked from commit 179d9ab) Co-authored-by: SoranaAurelia <[email protected]>

…11096) * added registers to layout in sabre_layout pass * Add reno and test * Fix layout method in test * Set transpiler seed * modified reference circuit for test --------- Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]>

added registers to layout in sabre_layout pass

1975483

SoranaAurelia requested a review from a team as a code owner October 23, 2023 23:14

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Oct 23, 2023

mtreinish added this to the 0.45.0 milestone Oct 30, 2023

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

jakelishman changed the title ~~Fix circuit drawer returning qubit[register] names for input (#11038)~~ Fix circuit drawer returning qubit[register] names for input Oct 30, 2023

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

Merge branch 'main' into issue_11038_draw_fix

ced0e3a

ElePT reviewed Nov 1, 2023

View reviewed changes

ElePT mentioned this pull request Nov 2, 2023

Prepare 0.45.0 release #11169

Merged

13 tasks

ElePT and others added 2 commits November 2, 2023 16:58

Add reno and test

9f5ee08

Merge branch 'main' into issue_11038_draw_fix

6630aff

jakelishman reviewed Nov 2, 2023

View reviewed changes

ElePT added 3 commits November 2, 2023 17:11

Fix layout method in test

55a1047

Merge branch 'issue_11038_draw_fix' of https://github.com/SoranaAurel…

d0a5c7a

…ia/qiskit into issue_11038_draw_fix

Set transpiler seed

2351394

jakelishman previously approved these changes Nov 2, 2023

View reviewed changes

jakelishman enabled auto-merge November 2, 2023 17:49

ElePT reviewed Nov 2, 2023

View reviewed changes

jakelishman modified the milestones: 0.45.0, 0.45.1 Nov 2, 2023

SoranaAurelia added 2 commits November 5, 2023 15:18

Merge branch 'main' into issue_11038_draw_fix

72da56a

modified reference circuit for test

3e990cf

auto-merge was automatically disabled November 5, 2023 13:36
Head branch was pushed to by a user without write access

SoranaAurelia dismissed jakelishman’s stale review via 3e990cf November 5, 2023 13:36

SoranaAurelia requested a review from jakelishman November 5, 2023 15:04

ElePT approved these changes Nov 6, 2023

View reviewed changes

ElePT added this pull request to the merge queue Nov 6, 2023

Merged via the queue into Qiskit:main with commit 179d9ab Nov 6, 2023

mergify bot mentioned this pull request Nov 6, 2023

Fix circuit drawer returning qubit[register] names for input (backport #11096) #11199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix circuit drawer returning qubit[register] names for input #11096

Fix circuit drawer returning qubit[register] names for input #11096

SoranaAurelia commented Oct 23, 2023

qiskit-bot commented Oct 23, 2023

coveralls commented Oct 31, 2023 •

edited

Loading

ElePT left a comment •

edited

Loading

ElePT left a comment

SoranaAurelia commented Nov 1, 2023

jakelishman Nov 2, 2023

jakelishman left a comment

ElePT left a comment

SoranaAurelia commented Nov 2, 2023

SoranaAurelia commented Nov 2, 2023

jakelishman commented Nov 2, 2023

SoranaAurelia commented Nov 2, 2023

jakelishman commented Nov 2, 2023

SoranaAurelia commented Nov 2, 2023

ElePT left a comment

Fix circuit drawer returning qubit[register] names for input #11096

Fix circuit drawer returning qubit[register] names for input #11096

Conversation

SoranaAurelia commented Oct 23, 2023

Summary

Details and comments

qiskit-bot commented Oct 23, 2023

coveralls commented Oct 31, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6761670924

Warning: This coverage report may be inaccurate.

💛 - Coveralls

ElePT left a comment • edited Loading

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

SoranaAurelia commented Nov 1, 2023

jakelishman Nov 2, 2023

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

SoranaAurelia commented Nov 2, 2023

SoranaAurelia commented Nov 2, 2023

jakelishman commented Nov 2, 2023

SoranaAurelia commented Nov 2, 2023

jakelishman commented Nov 2, 2023

SoranaAurelia commented Nov 2, 2023

ElePT left a comment

Choose a reason for hiding this comment

coveralls commented Oct 31, 2023 •

edited

Loading

ElePT left a comment •

edited

Loading