Circuit viewer #308

scarrazza · 2021-01-11T21:53:24Z

Provides a text based circuit viewer, closes #145.

Example:

from qibo.models import QFT
c = QFT(5)
print(c.draw())

@stavros11 @igres26 before polishing the code could you please let me know your opinion?

codecov · 2021-01-11T21:54:36Z

Codecov Report

Merging #308 (b3cb2c3) into master (25bd944) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #308   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           59        59           
  Lines        11209     11290   +81     
=========================================
+ Hits         11209     11290   +81

Flag	Coverage Δ
unittests	`100.00% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/qibo/abstractions/circuit.py	`100.00% <100.00%> (ø)`
src/qibo/abstractions/gates.py	`100.00% <100.00%> (ø)`
src/qibo/tests/test_circuits.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25bd944...b3cb2c3. Read the comment docs.

stavros11

Thanks for implementing this, I believe it is simple and provides a good visualization. Some comments regarding appearance:

Multi-control gates (eg. gates.X(0).controlled_by(1, 2, 3, 4)) do not appear properly. Specifically only the first control qubit is considered while the rest are ignored.
We could consider using capital letters for the gate representation (eg. X, Y, Z, H, I, U1, etc.), simlar to Cirq.
The controlled gate dot/bullet symbol appears a bit weird in my case:

so we may want to check compatibility with different terminal versions.
Although it may complicate the code slightly, I believe it will be better intuitively if we remove the c from the controlled gate names for the visualization. Since we explicitly show the control qubits using the dot symbols, we don't need the c in the target qubit symbol.
Currently the | symbol that connects the dot with the gate for controlled gates is not used when the control and target qubits are neighbors (for example see second cx in my figure). Again this would complicate the code, but it could look better if we used empty lines between qubits and use the | symbol for neighboring qubits too. In this case the matrix would have 2 * nqubits - 1 rows where every other row is empty or has | symbols.

stavros11 · 2021-01-12T09:32:53Z

src/qibo/abstractions/gates.py

@@ -12,7 +12,7 @@
              "cx": "CNOT", "swap": "SWAP", "cz": "CZ",
              "crx": "CRX", "cry": "CRY", "crz": "CRZ",
              "cu1": "CU1", "cu3": "CU3",
-              "ccx": "TOFFOLI"}
+              "ccx": "TOFFOLI", "id": "identity"}


If I remember correctly QASM_GATES is a map from the Qasm name of the gate to the corresponding Qibo class name, so this should be "id": "I". However I think that "id" is not a valid Qasm gate anyway so this entry probably should not be here.

I did a quick check and id seems to work, at least using the ibm q editor.

stavros11 · 2021-01-12T09:34:04Z

src/qibo/abstractions/gates.py

@@ -169,7 +169,7 @@ class I(Gate):

    def __init__(self, *q):
        super(I, self).__init__()
-        self.name = "identity"
+        self.name = "id"


We could also use the name I to be compatible with X, Y, Z and H.

stavros11 · 2021-01-12T09:43:33Z

src/qibo/abstractions/circuit.py

+            else:
+                t1 = targets[0]
+                matrix[t1][idx[t1]] = f"{gate.name}"
+                idx[t1] += 1


A small technical remark: The matrix that is created here is almost the "transpose" of self.queue.moments that already exists in the circuit, so there is a chance to simplify this with something like:

matrix = [[moment[i].name if moment[i] is not None else "" for moment in self.queue.moments] for i in range(self.nqubits)]

The issue is that for controlled gates this will put the gate.name for both the control and the target qubit so we would still need a mechanism to replace the control qubit name with the dot. Perhaps this could be handled in a gate method to simplify things here.

stavros11 · 2021-01-12T09:56:19Z

src/qibo/abstractions/circuit.py

+                        if iq in (qi, qf):
+                            if gate.name in ('id', 'collapse'):
+                                matrix[iq][column] = f"{gate.name}"
+                            elif gate.name in ('swap', 'fsim', 'generalizedfsim'):


This will use the X--X symbol for the fSim gate which is not correct as the fSim is not a SWAP. Perhaps we should include fSim and GenarlizedfSim to the previous if case where matrix[iq][column] = f'{gate.name}'.

scarrazza · 2021-01-12T10:35:03Z

Thanks for the comments, I agree with all of them, except for the "|" for neighbours, I think the plot is cleaner/more compact if we avoid an extra empty line with these connections, what do you think?

stavros11 · 2021-01-12T10:49:10Z

Thanks for the comments, I agree with all of them, except for the "|" for neighbours, I think the plot is cleaner/more compact if we avoid an extra empty line with these connections, what do you think?

Personally I am fine with either approach, I just proposed the other one because it is what Cirq uses. They also use @ instead of the bullet which may be universal in terms of different terminal versions. Perhaps @igres26 has some more concrete preferences regarding these choices.

igres26 · 2021-01-13T05:55:39Z

Looks good to me.

Having | between neighboring qubits might not be necessary since the main use of this feature is to display the circuit to someone who will be familiar, and not go to a paper. And it will be more compact.

I am not sure if using @ instead of dots might make it more universal, but we can look into that if that would fix the issue on Stavros' device.

I also agree with Stavros that only x instead of cx is enough when the controls are visible.

Does it work with all gates from Qibo or it only supports QASM gates?

Overall I think this is useful.

scarrazza · 2021-01-15T22:04:47Z

@stavros11 @igres26 I have implemented the features we have discussed last week, could you please have a look/test the code?

@stavros11 for simplicity, I decided to avoid the moments construction, I believe the current version is much more readable and easy to follow, let me know.

stavros11

Thanks for the updates, it looks good to me. No problem about not using the moments, I agree that the code is easy to follow as it is. The only two points I noticed are the following:

Perhaps we could expand the testing of this a bit, for example the case where line_wrap is not None is not tested. That being said, I played with various gate cases and combinations as well as the line_wrap and everything seems to work as expected.
When using the line_wrap, every chunk gets appended with ... in the beginning and the end. This also happens for the last chunk, however I believe we do not need ... at the end for the last chunk.

Some additional comments which are probably less important:

Although as I said it is easy to follow, it may be easier to test (if we extend tests) if we split this to a few auxiliary functions, instead of a big function as it is now. This point may require additional work and won't change anything in practice so I don't believe it is mandatory for now.
Depending on the line_wrap one selects, some gates that involve more than one character (eg. NOT) may get split in different chunks (eg. N and OT) which looks weird. Personally, I don't think we should do anything about this, just leave it up to the user to play and select a good line_wrap for each case. Most gates are single character anyway so this is not a big issue.

stavros11 · 2021-01-16T09:19:20Z

src/qibo/abstractions/circuit.py

+        idx = [0] * self.nqubits
+
+        for gate in self.queue:
+            gate_name = labels.get(gate.name)


There are some gates such as the CallbackGate, Flatten and the channels which are not included in labels. In this case using labels.get here will return a None which will raise some errors later which may be hard for the user to understand. I would suggest to include a check like:

if gate.name not in labels: raise_error(NotImplementedError, f"{gate.name} gate is not supported by `circuit.draw`")

stavros11 · 2021-01-16T09:23:12Z

src/qibo/abstractions/circuit.py

+        labels = {"h": "H", "x": "X", "y": "Y", "z": "Z",
+                  "rx": "RX", "ry": "RY", "rz": "RZ",
+                  "u1": "U1", "u2": "U2", "u3": "U3",
+                  "cx": "NOT", "swap": "x", "cz": "Z",


Suggested change

"cx": "NOT", "swap": "x", "cz": "Z",

"cx": "X", "swap": "x", "cz": "Z",

I believe X is more commonly used than NOT when plotting quantum circuits, even for the CNOT gate (despite calling it CNOT). If we use X we are also more consistent with how we plot TOFFOLI and multi-control X.

stavros11 · 2021-01-16T09:25:24Z

src/qibo/abstractions/circuit.py

+
+            # extend matrix
+            for iq in range(self.nqubits):
+                matrix[iq] += [ '' for _ in range(1 + col - len(matrix[iq]))]


I believe the following is slightly more efficient because it doesn't recreate the list:

Suggested change

matrix[iq] += [ '' for _ in range(1 + col - len(matrix[iq]))]

matrix[iq].extend((1 + col - len(matrix[iq]))* [''])

scarrazza · 2021-01-16T12:50:15Z

@stavros11, thanks for the comments, please let me know if you agree with the current changes. Concerning the line_wrap, I also think that the user can check what is better for his circuit setup, now, concerning the split in multiple function, I think we can avoid that with the current tests, but let me know your opinion.

stavros11 · 2021-01-17T09:52:30Z

Thanks for the update. The tests are okay and I don't think splitting the function is really needed, I just observed that there is still a problem with ... when using the line_wrap when the resulting chunks are more than two. For example

circuit = QFT(5)
print(circuit.draw(line_wrap=30))

gives

q0: ─H─U1─U1─U1─U1──────────── ...
q1: ───o──|──|──|──H─U1─U1─U1─ ...
q2: ──────o──|──|────o──|──|── ...
q3: ─────────o──|───────o──|── ...
q4: ────────────o──────────o── ...

... ───────────────x───
... ───────────────|─x─
... H─U1─U1────────|─|─
... ──o──|──H─U1───|─x─
... ─────o────o──H─x───

which is okay, but with line_wrap=20 it gives

q0: ─H─U1─U1─U1─U1── ...
q1: ───o──|──|──|──H ...
q2: ──────o──|──|─── ...
q3: ─────────o──|─── ...
q4: ────────────o─── ...

... ────────────────────
... ─U1─U1─U1───────────
... ─o──|──|──H─U1─U1───
... ────o──|────o──|──H─
... ───────o───────o────

... ─────x─── ...
... ─────|─x─ ...
... ─────|─|─ ...
... U1───|─x─ ...
... o──H─x─── ...

which is wrong because the second chunk does not have ... while the last one has. The same happens in any case where the chunks are more than two, I think the ... are always removed from the second chunk.

scarrazza · 2021-01-17T13:56:47Z

@stavros11 many thanks, this should be fixed now!

stavros11

@stavros11 many thanks, this should be fixed now!

Yes, everything seems to work as expected now.

scarrazza added 3 commits January 11, 2021 22:21

adding identity to qasm

ec7e6c2

adding first attempt of text circuit draw

db07531

adjusting output

41059eb

scarrazza requested review from stavros11 and igres26 January 11, 2021 21:53

stavros11 reviewed Jan 12, 2021

View reviewed changes

scarrazza added 7 commits January 15, 2021 11:18

adjusting symbols

949546e

adding controlled_by support

70dd1d4

fixing spacing

7997a1f

simplifying

8f4578a

adding documentation

189c942

adding test

784fe15

improving plotting algorithm

474d8ce

scarrazza changed the title ~~[WIP] circuit viewer~~ Circuit viewer Jan 15, 2021

stavros11 reviewed Jan 16, 2021

View reviewed changes

scarrazza added 3 commits January 16, 2021 13:43

fixing line_wrap

8c86c53

adding line wrap test

1c4a800

adding test for not supported gates

20f705a

fixing multi line wrap

b3cb2c3

stavros11 approved these changes Jan 17, 2021

View reviewed changes

scarrazza merged commit cbfaead into master Jan 17, 2021

scarrazza deleted the circuitdraw branch February 6, 2021 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Circuit viewer #308

Circuit viewer #308

scarrazza commented Jan 11, 2021

codecov bot commented Jan 11, 2021 •

edited

Loading

stavros11 left a comment

stavros11 Jan 12, 2021

scarrazza Jan 12, 2021

stavros11 Jan 12, 2021

stavros11 Jan 12, 2021

stavros11 Jan 12, 2021

scarrazza commented Jan 12, 2021

stavros11 commented Jan 12, 2021

igres26 commented Jan 13, 2021

scarrazza commented Jan 15, 2021

stavros11 left a comment

stavros11 Jan 16, 2021

stavros11 Jan 16, 2021

stavros11 Jan 16, 2021

scarrazza commented Jan 16, 2021

stavros11 commented Jan 17, 2021

scarrazza commented Jan 17, 2021

stavros11 left a comment

	"cx": "NOT", "swap": "x", "cz": "Z",
	"cx": "X", "swap": "x", "cz": "Z",

	matrix[iq] += [ '' for _ in range(1 + col - len(matrix[iq]))]
	matrix[iq].extend((1 + col - len(matrix[iq]))* [''])

Circuit viewer #308

Circuit viewer #308

Conversation

scarrazza commented Jan 11, 2021

codecov bot commented Jan 11, 2021 • edited Loading

Codecov Report

stavros11 left a 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

Choose a reason for hiding this comment

scarrazza commented Jan 12, 2021

stavros11 commented Jan 12, 2021

igres26 commented Jan 13, 2021

scarrazza commented Jan 15, 2021

stavros11 left a 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

scarrazza commented Jan 16, 2021

stavros11 commented Jan 17, 2021

scarrazza commented Jan 17, 2021

stavros11 left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 11, 2021 •

edited

Loading