Add display of internal circuits for ControlFlowOps to text drawer #10414

enavarro51 · 2023-07-11T16:22:54Z

Summary

Displays the circuits inside ControlFlowOps instead of just displaying a box.

Details and comments

This PR adds displays of internal circuits for ControlFlowOps to the circuit text drawer. This is similar to the capability in the mpl drawer in #10207. The main differences are,

In order to not recreate the wheel, the basic structure of the text drawer was maintained. Doing this meant it would have been difficult to draw complete boxes around the circuits and have them wrap onto multiple lines in the way they were done in the mpl drawer. Instead the flow circuits are bracketed at the beginning and end. See examples below.
When nesting control flow ops inside others, the mpl drawer used colors to indicate where the nesting was taking place. For the text drawer, a nesting number was used. So If-1, End-1 would appear inside If-0, End-0.

Otherwise all the previous functionality of the text drawer is intact. This PR incorporates the changes to utils.py that were made in #10207.

Add tests for text drawer
Release note

qr = QuantumRegister(5, "q")
cr = ClassicalRegister(3, "cr")
qc = QuantumCircuit(qr, cr)

qc.h(0)
qc.h(1)

with qc.if_test((cr, 1)):
    qc.y(1)
    qc.x(2)
    qc.z(3)

      ┌───┐                       
 q_0: ┤ H ├───────────────────────
      ├───┤┌─────  ┌───┐  ──────┐ 
 q_1: ┤ H ├┤       ┤ Y ├        ├─
      └───┘│       ├───┤        │ 
 q_2: ─────┤ If-0  ┤ X ├  End-0 ├─
           │       ├───┤        │ 
 q_3: ─────┤       ┤ Z ├        ├─
           └──╥──  └───┘  ──────┘ 
 q_4: ────────╫───────────────────
              ║                   
cr_0: ════════■═══════════════════
              ║                   
cr_1: ════════o═══════════════════
              ║                   
cr_2: ════════o═══════════════════
             0x1

qr = QuantumRegister(4, "q")
cr = ClassicalRegister(3, "cr")
qc = QuantumCircuit(qr, cr)

a = Parameter("a")
qc.h(0)
qc.measure(0, 2)
with qc.for_loop((2, 4, 8, 16), loop_parameter=a):
    qc.h(0)
    qc.cx(0, 1)
    qc.rx(pi / a, 1)
    qc.measure(0, 0)
    with qc.if_test((cr[2], 1)):
        qc.z(0)
qc.draw("text")

      ┌───┐┌─┐┌────────────────────  ┌───┐                ┌─┐┌────── ┌───┐»
 q_0: ┤ H ├┤M├┤                      ┤ H ├──■─────────────┤M├┤ If-1  ┤ Z ├»
      └───┘└╥┘│ For-0 (2, 4, 8, 16)  └───┘┌─┴─┐┌─────────┐└╥┘└──╥─── └───┘»
 q_1: ──────╫─┤                      ─────┤ X ├┤ Rx(π/a) ├─╫────╫─────────»
            ║ └────────────────────       └───┘└─────────┘ ║    ║         »
 q_2: ──────╫──────────────────────────────────────────────╫────╫─────────»
            ║                                              ║    ║         »
 q_3: ──────╫──────────────────────────────────────────────╫────╫─────────»
            ║                                              ║    ║         »
cr_0: ══════╬══════════════════════════════════════════════╩════╬═════════»
            ║                                                   ║         »
cr_1: ══════╬═══════════════════════════════════════════════════╬═════════»
            ║                                                   ║         »
cr_2: ══════╩═══════════════════════════════════════════════════■═════════»
                                                                          »
«       ───────┐   ──────┐ 
« q_0:   End-1 ├─        ├─
«       ───────┘   End-0 │ 
« q_1: ──────────        ├─
«                  ──────┘ 
« q_2: ────────────────────
«                          
« q_3: ────────────────────
«                          
«cr_0: ════════════════════
«                          
«cr_1: ════════════════════
«                          
«cr_2: ════════════════════
«

qiskit-bot · 2023-07-11T16:22:58Z

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

@enavarro51
@Qiskit/terra-core

coveralls · 2023-07-11T17:27:45Z

Pull Request Test Coverage Report for Build 5860928068

214 of 221 (96.83%) changed or added relevant lines in 1 file are covered.
14 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.01%) to 87.266%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/visualization/circuit/text.py	214	221	96.83%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	90.63%
crates/qasm2/src/parse.rs	12	96.67%

Totals
Change from base Build 5860553430:	0.01%
Covered Lines:	74442
Relevant Lines:	85305

💛 - Coveralls

…a into display_text_flow

enavarro51 · 2023-07-19T20:57:19Z

@jakelishman I updated the text drawer to deal with getting the circuits from node.op.blocks and proper usage of the inner_wire_map so it works as the mpl drawer does. cc8b7b7

…a into display_text_flow

enavarro51 · 2023-08-14T20:12:31Z

Update: The #10601 issue for the mpl drawer has now been fixed for the text drawer as well. An additional test was added.

…a into display_text_flow

enavarro51 · 2023-08-21T16:36:26Z

@jakelishman This latest update incorporates the changes to the mpl drawer in #10602. This included expanding the use of the wire_map and refactoring the conditional section.

jakelishman

I'm super super excited to have this in, thanks so much for working on it!

We spoke about this offline, but for posterity for others: I noticed this edge case produces unexpected output for me - an empty control-flow op doesn't print at all, but I'd still expect to see the brackets appear.

from qiskit import QuantumCircuit, ClassicalRegister, QuantumRegister

cr = ClassicalRegister(2)
qc = QuantumCircuit(QuantumRegister(2), cr)
with qc.if_test((cr, 0)):
   pass
qc.draw()

q3_0: ─

q3_1: ─

c6: 2/═

We then realised that this is because an empty control-flow block acts on zero qubits, so the problem is part of a larger issue around visualisation of zero-qubit instructions (#9962) and there's no action needed in this PR.

jakelishman · 2023-08-22T15:52:56Z

qiskit/visualization/circuit/text.py

+# Indicators for left, middle, and right of control flow gates
+CF_LEFT = 0
+CF_MID = 1
+CF_RIGHT = 2
+


Python's got a standard-library type enum.Enum for things like this in general, but it's probably not worth bothering changing here.

jakelishman · 2023-08-22T15:55:48Z

qiskit/visualization/circuit/text.py

+class FlowOnQuWire(DrawElement):
+    """Draws a box for a ControlFlowOp using a single qubit."""
+
+    def __init__(self, label="", top_connect="─", conditional=False, section=CF_LEFT):


Minor style: here and in the other new classes, it feels a bit weird to have section be optional; I don't think there's any default value that makes sense and the outer context should always known whether it's left/mid/right.

Done in 9b82cd7.

jakelishman · 2023-08-22T15:56:53Z

qiskit/visualization/circuit/text.py

+        if section == CF_RIGHT:
+            self.top_format = " ─%s─┐"
+            self.mid_format = "  %s ├"
+            self.bot_format = " ─%s─┘"
+        else:
+            self.top_format = "┌─%s─ "
+            self.mid_format = "┤ %s  "
+            self.bot_format = "└─%s─ "


Minor: how do you think it'd look if we had "mid" boxes display neither left nor right edge, so they appear more like a full continuation? At the moment it looks a shade unbalanced to me that we get more opening "brackets" than closing ones for if/else and switch constructs.

After several tries, decided this is fraught with lots of formatting issues, so left it with left edges.

Fine by me - it's all a bit of bike-shedding anyway.

jakelishman · 2023-08-22T16:00:05Z

qiskit/visualization/circuit/text.py

+            # In case there is an exception, this prevents it from printing twice.
+            self._single_string = " "


Tbh I'd probably skip this; these exceptions aren't actionable by a user, and an exception here would always indicate either an invalid circuit or a problem in our code, and we don't need to be too worried about optimising the return speed in those cases. I'm worried that setting to the empty string implies there's some meaning to it.

Done in 9b82cd7.

jakelishman · 2023-08-22T16:52:18Z

test/python/visualization/test_circuit_text_drawer.py

        """Test the wire_order option"""
        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/════════════",
-                "                    ",
+                "                  ",
+                "q_2: |0>──────────",
+                "        ┌───┐     ",
+                "q_1: |0>┤ X ├─────",
+                "        ├───┤┌───┐",
+                "q_3: |0>┤ H ├┤ X ├",
+                "        ├───┤└─╥─┘",
+                "q_0: |0>┤ H ├──╫──",
+                "        └───┘  ║  ",
+                " c_2: 0 ═══════o══",
+                "               ║  ",
+                "ca_0: 0 ═══════╬══",
+                "               ║  ",
+                "ca_1: 0 ═══════╬══",
+                "               ║  ",
+                " c_1: 0 ═══════■══",
+                "               ║  ",
+                " c_0: 0 ═══════o══",
+                "               ║  ",
+                " c_3: 0 ═══════■══",
+                "              0xa ",
            ]
        )
+


This test of wire_order looks unrelated; how come it needs changing?

You're right. I think I had an early problem with wire_order and cregbundle off, but that went away. I'll change it back.

Actually, I think there is a fundamental problem with wire_order changes to clbits and cregbundle on. How do you display the order if you have 3 qubits, and two 2-clbit registers with a wire order of [0, 2, 1, 6, 3, 4, 5]? I noticed that the tests for wire_order that are done in test_circuit_visualization only reorder the qubits. The reason this is an issue now is that with the control flow changes, the wire_map is used much more to access bit position than for example, self.qubits or self.clbits.

My suggestion would be to force cregbundle off if clbits are reordered.

Actually (actually) I just realized this is that crazy problem with the fact that the text drawer tests unlike the other drawers bypass the call to circuit_drawer which is where all the checks to force cregbundle false are done.

The reality is that this is a bad test, since cregbundle would normally be forced off by circuit_drawer before the call to the text drawer. So I say, either ditch the test or do what I did here, which is explicitly make cregbundle false.

So left this with cregbundle false which is the only way it should be tested. To avoid problems in the future, I think it's worthwhile to switch to using circuit_drawer for text_drawer testing.

Yep, 100% agree. Let's make that change in a separate PR with no feature changes, though, since it's a large-scale change to the testing structure.

…a into display_text_flow

jakelishman

This looks great - I'm super keen to get this merged and in so we can start using it, if you're also happy with it.

jakelishman · 2023-08-29T16:06:50Z

qiskit/visualization/circuit/text.py

-    def controlled_wires(node, layer):
+    def controlled_wires(node, wire_map):
        """
        Analyzes the node in the layer and checks if the controlled arguments are in
        the box or out of the box.

        Args:
            node (DAGNode): node to analyse
-            layer (Layer): The layer in which the node is inserted.
+            wire_map (dict): map of qubits/clbits to position


Oh, you managed to get this working? I'm pleased to see this - it looks generally more robust with the new idea of scopes.

…iskit#10414) * Update text.py * Update utils.py * Fix test using text * Lint * Lint * Fix if with body bug and first tests * Finish tests * Reno * Output=text * Cleanup * Fix repr and repr_html issue with jupyter * Cache _single_string to fix __repr__ and _repr_html__ issue * Get circuits from blocks and fix inner_wire_map * Fix gate on qubit issue and add test * Lint * First work on transpiled nested * Fix condition for flow ops * Intermediate flow_wire_map testing * Fix clbit bug * Finish transpiled bits bug * Final fix for conditions and transpiles * Fix review issues 1 * Revert wire_order test * Fix control gates with wire_map and test changes * Further case and else * Fix review items 2

enavarro51 added 3 commits July 11, 2023 08:49

Update text.py

15911e7

Update utils.py

777d271

Fix test using text

3918352

enavarro51 requested review from a team and nonhermitian as code owners July 11, 2023 16:22

Lint

26e2617

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

mtreinish added this to the 0.45.0 milestone Jul 11, 2023

Lint

de607b3

enavarro51 added 16 commits July 12, 2023 13:10

Fix if with body bug and first tests

de9f83d

Merge branch 'main' into display_text_flow

2f3379a

Finish tests

cdac918

Reno

c6a1682

Merge branch 'main' into display_text_flow

fa369a3

Merge branch 'main' into display_text_flow

dbd64e0

Output=text

925ad24

Merge branch 'main' into display_text_flow

2542ffe

Cleanup

7f6ecee

Merge branch 'display_text_flow' of github.com:enavarro51/qiskit-terr…

f34eaec

…a into display_text_flow

Fix repr and repr_html issue with jupyter

dba3b50

Cache _single_string to fix __repr__ and _repr_html__ issue

a4ff5f0

Merge branch 'main' into display_text_flow

733bce4

Get circuits from blocks and fix inner_wire_map

cc8b7b7

Merge branch 'display_text_flow' of github.com:enavarro51/qiskit-terr…

254f162

…a into display_text_flow

Merge branch 'main' into display_text_flow

96805bd

Merge branch 'main' into display_text_flow

6f890fd

enavarro51 added 4 commits August 13, 2023 08:53

Merge main and conflicts

26bf658

Fix gate on qubit issue and add test

b068b31

Merge branch 'display_text_flow' of github.com:enavarro51/qiskit-terr…

ea37e85

…a into display_text_flow

Merge branch 'main' into display_text_flow

afadd76

enavarro51 added 11 commits August 14, 2023 14:36

Lint

9db06ca

Merge branch 'main' into display_text_flow

5f87631

Merge branch 'display_text_flow' of github.com:enavarro51/qiskit-terr…

2c7e4d2

…a into display_text_flow

First work on transpiled nested

952f5d6

Fix condition for flow ops

c0ae2ca

Intermediate flow_wire_map testing

d51f8c6

Fix clbit bug

f3943f9

Merge branch 'main' into display_text_flow

72addb9

Finish transpiled bits bug

15bd398

Final fix for conditions and transpiles

e23186e

Merge branch 'main' into display_text_flow

b470233

jakelishman reviewed Aug 22, 2023

View reviewed changes

enavarro51 added 6 commits August 24, 2023 07:24

Fix review issues 1

2445160

Revert wire_order test

a552a0a

Fix control gates with wire_map and test changes

d8eda33

Further case and else

a9108ce

Fix review items 2

9b82cd7

Merge branch 'display_text_flow' of github.com:enavarro51/qiskit-terr…

ff488fb

…a into display_text_flow

jakelishman approved these changes Aug 29, 2023

View reviewed changes

jakelishman added this pull request to the merge queue Aug 29, 2023

Merged via the queue into Qiskit:main with commit 0a0e262 Aug 29, 2023

enavarro51 mentioned this pull request Sep 9, 2023

Fix issue with ControlFlowOps and cregbundle and mods to circuit drawer testing #10804

Closed

		# In case there is an exception, this prevents it from printing twice.
		self._single_string = " "

Add display of internal circuits for ControlFlowOps to text drawer #10414

Add display of internal circuits for ControlFlowOps to text drawer #10414

Uh oh!

Conversation

enavarro51 commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Jul 11, 2023

Uh oh!

coveralls commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 5860928068

💛 - Coveralls

Uh oh!

enavarro51 commented Jul 19, 2023

Uh oh!

enavarro51 commented Aug 14, 2023

Uh oh!

enavarro51 commented Aug 21, 2023

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

enavarro51 commented Jul 11, 2023 •

edited

Loading

coveralls commented Jul 11, 2023 •

edited

Loading

jakelishman Aug 29, 2023 •

edited

Loading