Relax wire_order restrictions in circuit visualization #9893

1ucian0 · 2023-04-01T14:04:16Z

Drawing circuits with wire_order has some restrictions, coming from the original design that, I think, can be relaxed a bit. For example if wire_order only refers to qubits, then cregbundle can be preserver. So this PR relax those restrictions and allows you to write wire_order=[2, 3, 0, 1] and cregbundle=True in a 4 qubit circuit, for example:

from qiskit import QuantumCircuit, QuantumRegister, ClassicalRegister

qr = QuantumRegister(4, "q")
cr = ClassicalRegister(4, "c")
cr2 = ClassicalRegister(2, "ca")
circuit = QuantumCircuit(qr, cr, cr2)
circuit.h(0)
circuit.h(3)
circuit.x(1)
circuit.x(3).c_if(cr, 10)

for method in ['text', 'mpl', 'latex']:
    display(circuit.draw(method, wire_order=[2, 3, 0, 1], cregbundle=True))

qiskit-bot · 2023-04-01T14:04:20Z

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:

1ucian0 · 2023-04-01T15:20:25Z

qiskit/opflow/__init__.py

@@ -19,7 +19,7 @@

 Operators and State functions are the building blocks of Quantum Algorithms.

-A library for Quantum Algorithms & Applications is more than a collection of
+A library for Quantum Algorithms & Applications (QA&A) is more than a collection of


This is unrelated. I just saw it around and noticed that QA&A is not a very common acronym.

It'd probably be better to revert this, especially with opflow deprecated. It's a problem that's existed forever (and capitalising unexpected words in a sentence like this is a not-altogether-unheard-of way of implicitly defining an acronym anyway).

done in b94ac8d

coveralls · 2023-04-01T15:45:58Z

Pull Request Test Coverage Report for Build 5542464288

10 of 11 (90.91%) changed or added relevant lines in 1 file are covered.
16 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.01%) to 85.989%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/visualization/circuit/circuit_visualization.py	10	11	90.91%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	91.14%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
crates/qasm2/src/parse.rs	12	97.11%

Totals
Change from base Build 5541406727:	-0.01%
Covered Lines:	71700
Relevant Lines:	83383

💛 - Coveralls

ajavadia

looks great thanks for fixing this

jakelishman

I don't think this is a good idea. Allowing only a partial wire_map I think is very confusing behaviour, and not generally clear like the initial intent of the option - I personally would have expected that giving a partial wire_map would only draw those wires, if it was going to have non-error behaviour. Really though, it just looks like an error to me.

If you really want this behaviuor for wire_map, can we at least make the syntax [2, 3, ...] (i.e. with a literal Ellipsis)?

1ucian0 · 2023-04-03T13:51:49Z

I personally would have expected that giving a partial wire_map would only draw those

To the contrary (and probably because I'm too famiiar with the drawer), only drawing some wires makes little sense to me. (how do you express a CX(0,1) without drawing 0 or 1?).

I saw some numpy Ellipsis usage in slicing, but it is the first time I see it as an element. Is that a thing? I have no problem to add it.

jakelishman · 2023-04-03T14:03:30Z

We spoke about this in the dev meeting just now. The notes are:

Ali's original request was mostly just about allowing only the qubits to be specified with the clbits omitted, so he'd be on board with allowing wire_map to have two valid lengths: one with all the qubits and clbits, and one with only all the qubits. If you prefer that, that there's no need for the ....
Matthew also thought that specifying only a partial list of qubits would have a filtering behaviour.
If we do want to allow only partial specification of qubits (or partial specification of clbits), there was general support for using the ... literal as the marker.

Given this, I'd vote to go with option 1: the wire_order list has (at most) two valid lengths: "all bits" or "all qubits". It solves Ali's initial request and it doesn't require extra syntax. (Personally I was surprised to learn that wire_order applies to the clbits at all, and we didn't have separate order fields for qubits and clbits, but that ship has sailed.)

jakelishman · 2023-04-03T14:05:01Z

Oh, and about "how do you draw cx(0, 1) if you filter out qubit 1?": that point is mostly why I said "[partial specification] looks like an error", yeah.

…_order_restrictions

1ucian0 · 2023-07-10T09:09:14Z

Given this, I'd vote to go with option 1: the wire_order list has (at most) two valid lengths: "all bits" or "all qubits".

done in that way. Have a look.

jakelishman

The new logic looks good, thanks, and consistent with what I remember us discussing. Could you fix the tests so they test what they say they do?

jakelishman · 2023-07-11T11:42:32Z

releasenotes/notes/relax_wire_order_restrictions-ffc0cfeacd7b8d4b.yaml

+    .. code-block::
+
+      from qiskit import QuantumCircuit, QuantumRegister, ClassicalRegister
+
+      qr = QuantumRegister(4, "q")
+      cr = ClassicalRegister(4, "c")
+      cr2 = ClassicalRegister(2, "ca")
+      circuit = QuantumCircuit(qr, cr, cr2)
+      circuit.h(0)
+      circuit.h(3)
+      circuit.x(1)
+      circuit.x(3).c_if(cr, 10)
+      circuit.draw('text', wire_order=[2, 3], cregbundle=True)
+
+    .. parsed-literal::
+
+       q_2: ────────────
+            ┌───┐ ┌───┐
+       q_3: ┤ H ├─┤ X ├─
+            ├───┤ └─╥─┘
+       q_0: ┤ H ├───╫───
+            ├───┤   ║
+       q_1: ┤ X ├───╫───
+            └───┘┌──╨──┐
+       c: 4/═════╡ 0xa ╞
+                 └─────┘
+      ca: 2/════════════


I don't think this example will work after the new changes any more because of the partial wire order. We should replace it with one that assigns all qubits.

oops... fixed in d2224cd

jakelishman · 2023-07-11T11:43:17Z

test/python/visualization/test_circuit_text_drawer.py

+    def test_wire_order_only_qubits(self):
+        """Test the wire_order option with only qubits"""
+        expected = "\n".join(
+            [
+                "                    ",
+                "q_2: |0>────────────",
+                "        ┌───┐       ",
+                "q_1: |0>┤ X ├───────",
+                "        ├───┤ ┌───┐ ",
+                "q_3: |0>┤ H ├─┤ X ├─",
+                "        ├───┤ └─╥─┘ ",
+                "q_0: |0>┤ H ├───╫───",
+                "        └───┘┌──╨──┐",
+                " c: 0 4/═════╡ 0xa ╞",
+                "             └─────┘",
+                "ca: 0 2/════════════",
+                "                    ",
+            ]
+        )
+        qr = QuantumRegister(4, "q")
+        cr = ClassicalRegister(4, "c")
+        cr2 = ClassicalRegister(2, "ca")
+        circuit = QuantumCircuit(qr, cr, cr2)
+        circuit.h(0)
+        circuit.h(3)
+        circuit.x(1)
+        circuit.x(3).c_if(cr, 10)
+        self.assertEqual(
+            str(_text_circuit_drawer(circuit, wire_order=[2, 1, 3, 0, 4, 5, 6, 7, 8, 9])), expected
+        )


This test says its wire_order is only qubits, but it actually has all bits.

test removed. See #9893 (comment)

jakelishman · 2023-07-11T11:43:50Z

test/python/visualization/test_circuit_latex.py

+    def test_wire_order_only_qubits(self):
+        """Test the wire_order list with qubits only to latex drawer"""
+        filename = self._get_resource_path("test_wire_order_only_qubits.tex")
+        qr = QuantumRegister(4, "q")
+        cr = ClassicalRegister(4, "c")
+        cr2 = ClassicalRegister(2, "ca")
+        circuit = QuantumCircuit(qr, cr, cr2)
+        circuit.h(0)
+        circuit.h(3)
+        circuit.x(1)
+        circuit.x(3).c_if(cr, 12)
+        circuit_drawer(
+            circuit,
+            cregbundle=False,
+            wire_order=[2, 1, 3, 0, 4, 5, 6, 7, 8, 9],
+            filename=filename,
+            output="latex_source",
+        )
+        self.assertEqualToReference(filename)


This test says its wire_order is only qubits, but it's actually got all of them. It should probably be a test of what it says, and shouldn't include the clbits.

actually, these tests make little sense now. Removing them in 445489c and add those that cover the involved code in this PR.

jakelishman · 2023-07-11T11:44:58Z

qiskit/opflow/__init__.py

@@ -19,7 +19,7 @@

 Operators and State functions are the building blocks of Quantum Algorithms.

-A library for Quantum Algorithms & Applications is more than a collection of
+A library for Quantum Algorithms & Applications (QA&A) is more than a collection of


It'd probably be better to revert this, especially with opflow deprecated. It's a problem that's existed forever (and capitalising unexpected words in a sentence like this is a not-altogether-unheard-of way of implicitly defining an acronym anyway).

…_order_restrictions

…5851

jakelishman

This looks much better now, thanks. I think the API we've settled on is the right compromise between permissibility and not being surprising.

enavarro51 · 2023-07-13T17:54:18Z

Just a note for all concerned, when we use circuit_drawer for tests that are supposed to have "text" output, can we please put output="text" in the call for those of us who default to "mpl" or otherwise. Thanks.

1ucian0 added 4 commits April 1, 2023 11:42

first attempt

c172441

fixing

8d3e170

other drawers

487d2d7

revert test/ipynb/mpl_tester.ipynb

b0994c7

1ucian0 requested review from a team, nonhermitian, woodsp-ibm and ikkoham as code owners April 1, 2023 14:04

1ucian0 added 3 commits April 1, 2023 16:53

corner case

6adb48b

latex test

d1aeffb

reno

95095d9

1ucian0 commented Apr 1, 2023

View reviewed changes

ajavadia previously approved these changes Apr 2, 2023

View reviewed changes

ajavadia added the automerge label Apr 2, 2023

jakelishman added on hold Can not fix yet and removed automerge labels Apr 2, 2023

jakelishman requested changes Apr 2, 2023

View reviewed changes

jakelishman removed the on hold Can not fix yet label Apr 2, 2023

1ucian0 added 7 commits April 11, 2023 15:11

Merge branch 'main' into relax_wire_order_restrictions

1bbf5b7

Merge branch 'main' of github.com:Qiskit/qiskit-terra into relax_wire…

ac2ee27

…_order_restrictions

Merge branch 'main' of github.com:Qiskit/qiskit-terra into relax_wire…

8aa8904

…_order_restrictions

readjust based on Qiskit#9893 (comment)

2fb598f

default complete_wire_order

5416f70

test adjustmet

6a1f792

bug in length

f229974

1ucian0 added 2 commits July 10, 2023 04:01

readjust tests

08f109a

ref

202353e

1ucian0 dismissed ajavadia’s stale review via 202353e July 10, 2023 09:02

1ucian0 added 2 commits July 10, 2023 04:02

remove old ref

acc1b0d

new ref

c541c6b

1ucian0 added this to the 0.25.0 milestone Jul 10, 2023

mtreinish assigned jakelishman Jul 10, 2023

jakelishman reviewed Jul 11, 2023

View reviewed changes

1ucian0 added 4 commits July 13, 2023 11:33

Merge branch 'main' of github.com:Qiskit/qiskit-terra into relax_wire…

312273f

…_order_restrictions

revert https://github.com/Qiskit/qiskit-terra/pull/9893/files#r125961…

b94ac8d

…5851

remove tests because they do not cover the extended code

445489c

reno fix

d2224cd

jakelishman approved these changes Jul 13, 2023

View reviewed changes

jakelishman added Changelog: New Feature Include in the "Added" section of the changelog mod: visualization qiskit.visualization labels Jul 13, 2023

jakelishman added this pull request to the merge queue Jul 13, 2023

Merged via the queue into Qiskit:main with commit b06072d Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax wire_order restrictions in circuit visualization #9893

Relax wire_order restrictions in circuit visualization #9893

1ucian0 commented Apr 1, 2023 •

edited

Loading

qiskit-bot commented Apr 1, 2023

1ucian0 Apr 1, 2023

jakelishman Jul 11, 2023

1ucian0 Jul 13, 2023 •

edited

Loading

coveralls commented Apr 1, 2023 •

edited

Loading

ajavadia left a comment

jakelishman left a comment •

edited

Loading

1ucian0 commented Apr 3, 2023 •

edited

Loading

jakelishman commented Apr 3, 2023

jakelishman commented Apr 3, 2023

1ucian0 commented Jul 10, 2023

jakelishman left a comment

jakelishman Jul 11, 2023

1ucian0 Jul 13, 2023 •

edited

Loading

jakelishman Jul 11, 2023

1ucian0 Jul 13, 2023

jakelishman Jul 11, 2023

1ucian0 Jul 13, 2023

jakelishman Jul 11, 2023

jakelishman left a comment

enavarro51 commented Jul 13, 2023

Relax wire_order restrictions in circuit visualization #9893

Relax wire_order restrictions in circuit visualization #9893

Conversation

1ucian0 commented Apr 1, 2023 • edited Loading

qiskit-bot commented Apr 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ucian0 Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

coveralls commented Apr 1, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5542464288

💛 - Coveralls

ajavadia left a comment

Choose a reason for hiding this comment

jakelishman left a comment • edited Loading

Choose a reason for hiding this comment

1ucian0 commented Apr 3, 2023 • edited Loading

jakelishman commented Apr 3, 2023

jakelishman commented Apr 3, 2023

1ucian0 commented Jul 10, 2023

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ucian0 Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

enavarro51 commented Jul 13, 2023

1ucian0 commented Apr 1, 2023 •

edited

Loading

1ucian0 Jul 13, 2023 •

edited

Loading

coveralls commented Apr 1, 2023 •

edited

Loading

jakelishman left a comment •

edited

Loading

1ucian0 commented Apr 3, 2023 •

edited

Loading

1ucian0 Jul 13, 2023 •

edited

Loading