Open balopat opened 3 years ago
Another diagram nit: It's great to have the qubit map there. However, we could make it a bit cleaner.
Code:
import cirq
from cirq import *
a, b, c, d = GridQubit.rect(2,2)
q = [NamedQubit(i) for i in ["a","b","c"]]
op = CircuitOperation(FrozenCircuit(cirq.X(q[0]), cirq.Y(q[1]), cirq.Z(q[2]), cirq.CX(q[0], q[1]))).with_tags(["tag"])
print(Circuit(X(a), Y(b), Z(c), CX(a, b), H(c), op.with_qubits(a,b,d), [H(i) for i in [a,b,c,d]]))
Current output:
┌──────────────────────────────────────────────────────────────────────────────────┐
Circuit_0x740eb7df41d7eca6:
[ a: ───X───@─── ]
(0, 0): ───X───@────[ │ ]───────────────────────────────────────────────────────────H───
│ [ b: ───Y───X─── ]
│ [ ]
│ [ c: ───Z─────── ](qubit_map={a: (0, 0), b: (0, 1), c: (1, 1)})[['tag']]
│ │
(0, 1): ───Y───X────#2────────────────────────────────────────────────────────────────────────────────────H───
│
(1, 0): ───Z───H────┼────────────────────────────────────────────────────────────────────────────────H────────
│
(1, 1): ────────────#3────────────────────────────────────────────────────────────────────────────────────H───
└──────────────────────────────────────────────────────────────────────────────────┘
Improved:
┌─────────────────────────────────────┐
Circuit_0x740eb7df41d7eca6[['tag']]:
[ (0,0) -> a: ───X───@───]
(0, 0): ───X───@────[ │ ]──────────────H───
│ [ (0,1) -> b: ───Y───X───]
│ [ ]
│ [ (1,1) -> c: ───Z───────]
│ │
(0, 1): ───Y───X────#2───────────────────────────────────────H───
│
(1, 0): ───Z───H────┼───────────────────────────────────H────────
│
(1, 1): ────────────#3───────────────────────────────────────H───
└─────────────────────────────────────┘
_Originally posted by @balopat in https://github.com/quantumlib/Cirq/pull/3580#discussion_r542666148_
See #3605 for the (separate?) consideration of out-of-line subcircuit diagrams.
Child of #3235
Can I take this one?
Can I take this one?
Sure! I've assigned it to you. At first glance, this seems fairly clear-cut, but there may be some intricacies in how this interacts with the core circuit diagram drawing tools. I'm interested to see your solution!
Also pinging @daxfohl who has been making changes adjacent to this space - just want to make sure you're both aware of the work the other is doing.
Yes, following it (or, at least the previous diagramming PRs). I've merged and updated the classical control diagram tests in my branch accordingly.
I looked at the code a little over the weekend and I am thinking of doing this:
#2
│
┼
│
#3
What do you all think?
@daxfohl @95-martin-orion @balopat Implemented design laid out in previous comment here: https://github.com/quantumlib/Cirq/pull/4745 Please take a look.
Quick FYI -- balopat is no longer here (much), and I'm not a maintainer. But that aside, I think (1) sounds nice, though I think if a CircuitOperation has only one line it should still have a box. For (2), I think it sounds like a good idea except when printing out just the subcircuit, since then that information would get lost. If that special casing for (2) adds too much complexity I'd say it's probably not worth it.
@95-martin-orion Do you have any concern about breaking users who might have unit tests that check diagram output, especially in the context of (1) going beyond affecting CircuitOperation output? I can't think of a reason third-party users would do that, but you've seen more than I have.
@daxfohl 1) I think your point that you would like a box around a single line CircuitOperation is a good one. Let me think about it, but that might be enough to warrant just delegating the boxing to the operation. 2) I think that the mapping doesn't even make sense outside the context of a circuit. What are these magically qubits this operation is mapping too? :)
I'll be commenting on the PR as well, but I'd like to highlight here that I agree with @daxfohl on item (2). A CircuitOperation
combines a FrozenCircuit
with the mappings required to include it in a larger circuit. All of those mappings should be clearly visible when printing out the CircuitOperation
, whether it's in the context of that larger circuit or not. Think of the FrozenCircuit
as a function, and the CircuitOperation
as an invocation of that function - including arguments (mappings) passed into it.
The inline circuit diagram introduced in https://github.com/quantumlib/Cirq/pull/3580 is awesome. If we could push it a bit further that would be cool - eg using proper box ascii drawing. So currently what looks like this:
, instead could look like this
Even cooler would be pulling in the right side of the box...
_Originally posted by @balopat in https://github.com/quantumlib/Cirq/pull/3580#discussion_r542634790_