Qc render fix #3186

exAClior · 2020-08-02T14:10:29Z

Related Issue

Source of Error

In the original code, we added \qw and \qwx too aggressively. Whenever there is a line in the original diagram, we will try to insert \qw or \qwx accordingly. This causes the program to try to add the angle bracket thing above and below the two cnots mentioned in the issue to be recognized as wires.

Summary of Fix

Add the pruning process for adding \qw and \qwx. Need none wire item to be on the same row or previous row needs to be connected to the current one.

Output After Fix

Rerunning code outputs the following.

\Qcircuit @R=1em @C=0.75em {
 \\
 &                      &    &                &                &    & \\
 &\lstick{\text{0}}& \qw& \qw&\control \qw    &         \qw    & \qw&\qw\\
 &\lstick{\text{1}}& \qw& \qw&         \qw\qwx&\control \qw    & \qw&\qw\\
 &\lstick{\text{2}}& \qw& \qw&\targ    \qw\qwx&         \qw\qwx& \qw&\qw\\
 &\lstick{\text{3}}& \qw& \qw&         \qw    &\targ    \qw\qwx& \qw&\qw\\
 &                      &    &                &                &    & \\
 \\
}

Latex Compiled Output

Following are the Latex compiled output for the above code and one extra to show we don't over connect using \qwx

Misc

Seems to pass the qcircuit_test.py. But I fear that we might need other tests in the future? Please let me know if I did something wrong.

This reverts commit 54ad2cf.

balopat

Thank you for working on this!
I will have to look at the algo itself, but first it needs a test - I recommend reproducing the issue with the smallest possible example with a test that would fail without your fix. Then add in your fix. That should also remove the need for ignoring the coverage tool!

balopat · 2020-08-03T16:45:20Z

cirq/contrib/qcircuit/qcircuit_diagram.py

+        if row_has_item:
+            diagram2.write(2 * w - 1, y, r'&\qw\\')
+        else:
+            diagram2.write(2 * w - 1, y, r'& \\')  # coverage: ignore


We should only ignore coverage in very exceptional scenarios where it's impossible/very expensive to test. I don't think this is that case.

Thanks for the help! Sorry about the mistake here, I did not understand what the coverage tool was for fully. I suppose I would add the appropriate test into qcircuit_test.py? On a side note, I had some problems with not passing the formatting test. Is there a formatting tool everyone else is using for this project? I ran mypy and pylint with the appropriate configuration file. Neither of them complained about my coding style. Yet the formatting test gave me a hard time.

Yes, you should write a new test method in qcircuit_test.py. It will contain a test circuit and the expected output similar to this:

Cirq/cirq/contrib/qcircuit/qcircuit_test.py

Lines 110 to 123 in d848611

def test_other_diagram():

a, b, c = cirq.LineQubit.range(3)

circuit = cirq.Circuit(cirq.X(a), cirq.Y(b), cirq.Z(c))

expected_diagram = r"""

\Qcircuit @R=1em @C=0.75em {

\\

&\lstick{\text{0}}& \qw&\targ \qw&\qw\\

&\lstick{\text{1}}& \qw&\gate{\text{Y}} \qw&\qw\\

&\lstick{\text{2}}& \qw&\gate{\text{Z}} \qw&\qw\\

\\

}""".strip()

assert_has_qcircuit_diagram(circuit, expected_diagram)

Regarding formatting - you can just run check/format-incremental --apply and that will reformat your files properly.

btw feel free to jump on https://gitter.im/cirqdev/community if you want to chat

Thanks a lot for the help! 👍 I ended up using yapf directly. The ./format-incremental --apply did not really work for me. Perhaps it would be nice for me to mention that by the end of CONTRIBUTING.md? I felt really bad spamming these commits that fail the format testing. Perhaps it would help future newbie's like me who wants to contribute?

balopat

Thanks for the test! I added my comments, I think an alternative design can make this significantly simpler: we should just skip moment group generation for qcircuit. Unless there is a way to draw those brackets with qcircuit - which would be even cooler.

balopat · 2020-08-04T23:10:16Z

cirq/contrib/qcircuit/qcircuit_test.py

+    expected_diagram = r"""
+\Qcircuit @R=1em @C=0.75em {
+ \\
+ &                      &    &                &                &    & \\


we shouldn't need to have an empty line here

balopat · 2020-08-04T23:10:21Z

cirq/contrib/qcircuit/qcircuit_test.py

+ &\lstick{\text{1}}& \qw& \qw&         \qw\qwx&\control \qw    & \qw&\qw\\
+ &\lstick{\text{2}}& \qw& \qw&\targ    \qw\qwx&         \qw\qwx& \qw&\qw\\
+ &\lstick{\text{3}}& \qw& \qw&         \qw    &\targ    \qw\qwx& \qw&\qw\\
+ &                      &    &                &                &    & \\


we shouldn't need to have an empty line here either

balopat · 2020-08-04T23:12:09Z

cirq/contrib/qcircuit/qcircuit_diagram.py

+            # if has none wire before, should draw wire
+            post1 = r'\qw' if key in qw and row_has_item else ''
+            post2 = ''
+            if key in qwx:


This solution is interesting, but it is very hard to read and reason about.
The key issue is that the moment groups character is not handled well by qcircuit. So, I would rather introduce a by default True parameter to to_text_diagram_drawer, something like draw_moment_groups - and then the qcircuit drawer could set that to false.

Got it! I will get back to this ASAP but I just lost power here a couole hours ago. The storm hit us hard. Just a heads up that I might not be able to work at all in the next day or two

No worries, no rush - good luck with the storm, stay safe!

Hello @balopat, as I looked at your suggestion, I realized how silly my method is. I tried to keep the angle bracket to indicate the moment as I thought it is a really good reminder for viewers that these gates can be run simultaneously. But I hit a wall and got no good results. So, I basically did what you said, and added a test in qcircuit_test.py to account for the circuit that caused our initial problem #3183.
I have been trying to battle with the doc build error from CI but I did not even change the file it was mentioning. Then I went to change the file itself by just calling yapf on the mentioned code, it is still giving me an error. If you got time, could you please take a look at it? Thanks in advance for your time and help.

…efore

balopat · 2020-08-10T17:27:15Z

Hi @exAClior - I suggest fixing somehow your branch - you seemed to pull in some commits from master incorrectly - the 3 files that you changed look good, but Github shows 26 files changed somehow.

The doc failures are unrelated, other PRs are breaking on it too - I'll fix them soon.

@r

### Related Issue Fixes quantumlib#3183. ### Source of Error In the original code, we added \qw and \qwx too aggressively. Whenever there is a line in the original diagram, we will try to insert \qw or \qwx accordingly. This causes the program to try to add the angle bracket thing above and below the two cnots mentioned in the issue to be recognized as wires. ### Summary of Fix Add the pruning process for adding \qw and \qwx. Need none wire item to be on the same row or previous row needs to be connected to the current one. ### Output After Fix Rerunning code outputs the following. \Qcircuit @r=1em @C=0.75em { \\ & & & & & & \\ &\lstick{\text{0}}& \qw& \qw&\control \qw & \qw & \qw&\qw\\ &\lstick{\text{1}}& \qw& \qw& \qw\qwx&\control \qw & \qw&\qw\\ &\lstick{\text{2}}& \qw& \qw&\targ \qw\qwx& \qw\qwx& \qw&\qw\\ &\lstick{\text{3}}& \qw& \qw& \qw &\targ \qw\qwx& \qw&\qw\\ & & & & & & \\ \\ } ### Latex Compiled Output Following are the Latex compiled output for the above code and one extra to show we don't over connect using \qwx ![Screenshot_20200802_095335](https://user-images.githubusercontent.com/10986218/89124597-4675b400-d4a6-11ea-9fcb-44482ecdbb95.png) ![Screenshot_20200802_095331](https://user-images.githubusercontent.com/10986218/89124599-4675b400-d4a6-11ea-9bba-bf78de2bb9f1.png) ### Misc Seems to pass the qcircuit_test.py. But I fear that we might need other tests in the future? Please let me know if I did something wrong.

exAClior added 2 commits August 2, 2020 09:42

try to fix additional qw

dde3ff1

fix unable to pass test

eb5c03f

googlebot added the cla: yes Makes googlebot stop complaining. label Aug 2, 2020

exAClior added 7 commits August 2, 2020 10:44

fix for unable to pass test

4804def

fix for formatting and type issues

54ad2cf

Revert "fix for formatting and type issues"

626fefe

This reverts commit 54ad2cf.

try to fix style issue

294e0d9

should fix formatting

20b46cf

silly formatting miss

acd7914

not ehpasising arithm order

65677e4

balopat suggested changes Aug 3, 2020

View reviewed changes

exAClior added 2 commits August 3, 2020 13:41

removed converge ignore line and added test for that

f8b2303

formatted using yapf

fe66c59

balopat suggested changes Aug 4, 2020

View reviewed changes

exAClior added 6 commits August 8, 2020 14:09

add bool parameter to solve moment problem

f118598

fixed conflict with previous changes

e3fb055

try to fix doc build failure

2382935

really trying to fix the doc issue, I did not even modify this file b…

0bcb956

…efore

could it be the boolean been not ordered correctly?

14d9473

fix format issue

528225f

exAClior requested a review from balopat August 8, 2020 19:50

exAClior added 3 commits August 9, 2020 08:09

revert to before doc build fail appears

c5a9263

apply fix again

ecb27c4

added draw_moment_group into circuit.py

81363f0

Merge branch 'master' into qc-render-fix

2f22d46

balopat approved these changes Aug 10, 2020

View reviewed changes

balopat added automerge Tells CirqBot to sync and merge this PR. (If it's running.) and removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) labels Aug 10, 2020

balopat added automerge Tells CirqBot to sync and merge this PR. (If it's running.) and removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) labels Aug 11, 2020

exAClior closed this Aug 11, 2020

exAClior reopened this Aug 11, 2020

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 11, 2020

CirqBot merged commit 059006d into quantumlib:master Aug 11, 2020

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Aug 11, 2020

exAClior deleted the qc-render-fix branch August 11, 2020 18:57

	def test_other_diagram():
	a, b, c = cirq.LineQubit.range(3)

	circuit = cirq.Circuit(cirq.X(a), cirq.Y(b), cirq.Z(c))

	expected_diagram = r"""
	\Qcircuit @R=1em @C=0.75em {
	\\
	&\lstick{\text{0}}& \qw&\targ \qw&\qw\\
	&\lstick{\text{1}}& \qw&\gate{\text{Y}} \qw&\qw\\
	&\lstick{\text{2}}& \qw&\gate{\text{Z}} \qw&\qw\\
	\\
	}""".strip()
	assert_has_qcircuit_diagram(circuit, expected_diagram)

Qc render fix #3186

Qc render fix #3186

Uh oh!

Conversation

exAClior commented Aug 2, 2020 • edited by balopat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue

Source of Error

Summary of Fix

Output After Fix

Latex Compiled Output

Misc

Uh oh!

balopat 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!

balopat Aug 3, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

balopat 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!

exAClior Aug 8, 2020 • 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!

balopat commented Aug 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

exAClior commented Aug 2, 2020 •

edited by balopat

Loading

balopat Aug 3, 2020 •

edited

Loading

exAClior Aug 8, 2020 •

edited

Loading