Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Glitched Circuit Diagram after flatten #3196

Closed
AlkaidCheng opened this issue Aug 6, 2020 · 15 comments · Fixed by #3221
Closed

Glitched Circuit Diagram after flatten #3196

AlkaidCheng opened this issue Aug 6, 2020 · 15 comments · Fixed by #3221
Assignees
Labels
good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@AlkaidCheng
Copy link

AlkaidCheng commented Aug 6, 2020

Description of the issue
When a circuit has a parameterised gate as a product of symbols, flattening the circuit will produce a glitched circuit diagram where the parameterised expression is gone. This will further affect the circuit diagram if two qubit gates are also present in the circuit.
How to reproduce the issue

import sympy as sp
from cirq import CNOT, rz, Circuit
cq = cirq.Circuit()
qubits = cirq.GridQubit.rect(2,1)
x = sp.symarray('x', 2)
cq.append([rz(x[0]*x[1])(qubits[-1]), CNOT(*tuple(qubits))])
new_cq, _= cirq.flatten(cq)
new_cq

This will product circuit diagram like this

This will product circuit diagram like this

(0, 0): ─────────────────────────@───
           │
(1, 0): ───Rz(pi*)───X───

Cirq version
0.8.2

@balopat balopat added good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. and removed good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. labels Aug 6, 2020
@balopat
Copy link
Contributor

balopat commented Aug 6, 2020

Hi @AlkaidCheng - the result on the CLI is

(0, 0): ─────────────────────────@───
                                 │
(1, 0): ───Rz(pi*<x_0*x_1/pi>)───X───

I'm guessing that the angle brackets < > create the problem in a browser for you - maybe Jupyter notebooks?

@balopat balopat added the good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. label Aug 6, 2020
@balopat
Copy link
Contributor

balopat commented Aug 6, 2020

Yeah I just confirmed that that's the issue. In Jupyter notebooks this shows up incorrectly, but only if the object is returned as the result of the cell - if you use print, it works

print(new_cq)

Gives a workaround, but it might be a bit annoying. Not sure yet what the right solution is here.

@balopat balopat removed the good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. label Aug 6, 2020
@AlkaidCheng
Copy link
Author

Right, I was using Colab notebooks and doing print will solve the problem.

@AlkaidCheng
Copy link
Author

AlkaidCheng commented Aug 7, 2020

I have been using the circuit returned as the result of the cell for printing circuits because for jupyter notebooks, doing print(circuit) will mess up the diagram by introducing extra newlines if the circuit is too long. Interestingly doing print(circuit) in Colab notebooks does not have this problem.

@balopat balopat added difficulty: trivial good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on and removed difficulty: trivial labels Aug 13, 2020
@balopat
Copy link
Contributor

balopat commented Aug 13, 2020

For someone who wants to look at this should review the possibility of replacing <> angle brackets with another pair of symbols (e.g. {} or []) here: https://github.com/quantumlib/Cirq/blob/master/cirq/study/flatten_expressions.py

@abhik-99
Copy link
Contributor

Hey! Can I contribute to this issue?

@balopat
Copy link
Contributor

balopat commented Aug 13, 2020

Hi @abhik-99 - yes please, feel free to grab it!

@balopat
Copy link
Contributor

balopat commented Aug 13, 2020

After a quick internal chat - consensus is on using [ ].

@cduck
Copy link
Collaborator

cduck commented Aug 13, 2020

I haven't been following this issue but it sounds like the angle brackets are a problem in the Jupyter scrollable view because they are interpreted as HTML tags. An easy fix for this would be to escape any special characters in the code for Circuit._repr_html_ (probably with the builtin html module).

@balopat
Copy link
Contributor

balopat commented Aug 13, 2020

Oooo awesome suggestion @cduck, I didn't know about _repr_html_ - @abhik-99 - please try that out - if we could implement the escaping there that would be even better and actually more robust against future similar issues.

@abhik-99
Copy link
Contributor

Sure @balopat! On it. Thanks.

@mpharrigan
Copy link
Collaborator

xref #2905 <virtual> labels for noise tags also get eaten in jupyter sometimes

@abhik-99
Copy link
Contributor

abhik-99 commented Aug 15, 2020

So hey @balopat and @cduck I tried out two possible solutions.

  1. Replacing the < > with [ ] in https://github.com/quantumlib/Cirq/blob/master/cirq/study/flatten_expressions.py in here.
  2. Trying to escape using the builtin html module in https://github.com/quantumlib/Cirq/blob/master/cirq/circuits/circuit.py in here.

Solution 1 is the easiest one but solution 2 is better in preventing similar future issues. The thing is that in solution 2, as the &lt; and &gt; are being shown as < and >, there is a space that needs to be filled. Adding in spaces only enlarges the image width-wise. I have tried using cirq.circuits._text_diagram_drawer.TextDiagramDrawer.shift() but that only shifts the whole diagram not a single qubit line.

Any suggestions?

@cduck
Copy link
Collaborator

cduck commented Aug 15, 2020

It would be helpful to give the code you used for those examples. I would do the second solution by changing line https://github.com/quantumlib/Cirq/blob/master/cirq/circuits/circuit.py#L378 to + html.escape(...). This way circuit diagram will make space for < before it is replaced with the escape code.

@abhik-99
Copy link
Contributor

Thanks a lot @cduck ! Just did that and turns out it works great. Opening a PR on a side branch on the lint and tests pass.

cduck pushed a commit that referenced this issue Aug 18, 2020
This PR fixes #3196 by using the `html` module for escaping characters when the circuit diagram is produced. The escaping allows easy and correct rendering of the diagram when it is used as a cell output of a Jupyter Notebook instead of using `print(circuit)`.
Previsouly, the special characters like `< >` were not being rendered after the circuit was flattened as they were being interpreted as part of HTML tags. Using `html.escape( )` in `circuit._repr_html_(self)` prevents that from happening.
tonybruguier pushed a commit to tonybruguier/Cirq that referenced this issue Aug 23, 2020
This PR fixes quantumlib#3196 by using the `html` module for escaping characters when the circuit diagram is produced. The escaping allows easy and correct rendering of the diagram when it is used as a cell output of a Jupyter Notebook instead of using `print(circuit)`.
Previsouly, the special characters like `< >` were not being rendered after the circuit was flattened as they were being interpreted as part of HTML tags. Using `html.escape( )` in `circuit._repr_html_(self)` prevents that from happening.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants