CircuitOperation diagram styling

[balopat]

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:

0: ───X───@─────────────────────────────────
          │
          │   Circuit_0x0effa90e9371143d:
          │   [ 1: ───X───@───          ]
1: ───Y───X───[           │             ]───
              [ 2: ───Y───X───          ]
              [                         ]
              [ 3: ───Z───────          ]
              │
2: ───Z───────#2────────────────────────────
              │
3: ───────────#3────────────────────────────

, instead could look like this

0: ───X───@─────────────────────────────────
          │
          │   ┌Circuit_0x0effa90e9371143d┐
          │   │ 1: ───X───@───           │
1: ───Y───X───│           │              │───
              │ 2: ───Y───X───           │
              │                          │
              │ 3: ───Z───────           │
              ├──────────────────────────┘
2: ───Z───────#2────────────────────────────
              │
3: ───────────#3────────────────────────────

Even cooler would be pulling in the right side of the box...

0: ───X───@─────────────────────────────────
          │
          │   Circuit_0x0effa90e9371143d
          │   ┌────────────────┐
          │   │ 1: ───X───@─── │
1: ───Y───X───│           │    │───
              │ 2: ───Y───X─── │
              │                │
              │ 3: ───Z─────── │
              ├────────────────┘
2: ───Z───────#2────────────────────────────
              │
3: ───────────#3────────────────────────────

Originally posted by @balopat in https://github.com/quantumlib/Cirq/pull/3580#discussion_r542634790

[balopat]

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:

1. mapping in-place for qubits outer -> inner
2. tag(s) to the header instead

                   ┌─────────────────────────────────────┐        
                    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

[95-martin-orion]

See #3605 for the (separate?) consideration of out-of-line subcircuit diagrams.

[95-martin-orion]

Child of #3235

[thanacles]

Can I take this one?

[95-martin-orion]

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.

[daxfohl]

Yes, following it (or, at least the previous diagramming PRs). I've merged and updated the classical control diagram tests in my branch accordingly.

[thanacles]

I looked at the code a little over the weekend and I am thinking of doing this:

1. It seems like the concern of partitioning out multi-line operations from the rest of the circuit belongs at the circuit level. So I think we should refactor TextDiagramDrawer to put boxes around multi-line operations. This would entail removing box code from current operations and gates like MatrixGate. I think it would be worthwhile because it seems like we really want all multi-line operations to have boxes and this would be a good way to enforce it and it would reduce code duplication.
2. I think the qubit_map data is redundant as you get that information from line coming out of the CircuitOperation in the diagram. So I think we should just delete the qubit_map data from the diagram.

#2
│
┼
│
#3

What do you all think?

[thanacles]

@daxfohl @95-martin-orion @balopat Implemented design laid out in previous comment here: https://github.com/quantumlib/Cirq/pull/4745 Please take a look.

[daxfohl]

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.

[thanacles]

@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? :)

[95-martin-orion]

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.