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

Python: Long gate names are rendered poorly in ASCII art circuits #1476

Open
minestarks opened this issue May 3, 2024 · 9 comments
Open
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@minestarks
Copy link
Member

When rendering circuits (see https://github.com/microsoft/qsharp/blob/main/samples/notebooks/circuits.ipynb) , if the gate label is longer than our fixed column width (e.g. rx(3.1416)) then the resulting ASCII art circuit looks terrible.

We should make column widths vary with the gate label, so that the gates are displayed properly on the qubit wire.

See the below samples in Python.

import qsharp

print("gates that fit the column width")
print("circuit looks okay")

qsharp.eval("""

use q0 = Qubit();
use q1 = Qubit();

H(q0);
H(q1);
X(q1);     
CNOT(q0, q1);
M(q0)

""")

print(qsharp.dump_circuit())


print("q_2 has gates that are too wide for the column width")
print("circuit wires disappear, and the vertical columns don't align anymore")

qsharp.eval("""

use q2 = Qubit();

Rx(1.0, q2);
Rx(1.0, q2);

""")

print(qsharp.dump_circuit())


print("q_3 has gates with both short and long gate labels")
print("here, the column widths should be variable so that the H doesn't take up too much width, but rx(1.000) still fits within a column")

qsharp.eval("""

use q3 = Qubit();

H(q3);
Rx(1.0, q3);
H(q3);
Rx(1.0, q3);
H(q3);
Rx(1.0, q3);


""")

print(qsharp.dump_circuit())

Output:

gates that fit the column width
circuit looks okay
q_0    ── H ─────────── ● ──── M ──
                        │      ╘═══
q_1    ── H ──── X ──── X ─────────

q_2 has gates that are too wide for the column width
circuit wires disappear, and the vertical columns don't align anymore
q_0    ── H ─────────── ● ──── M ──
                        │      ╘═══
q_1    ── H ──── X ──── X ─────────
q_2     rx(1.0000)  rx(1.0000) ──────────────

q_3 has gates with both short and long gate labels
here, the column widths should be variable so that the H doesn't take up too much width, but rx(1.000) still fits within a column
q_0    ── H ─────────── ● ──── M ────────────────
                        │      ╘═════════════════
q_1    ── H ──── X ──── X ───────────────────────
q_2     rx(1.0000)  rx(1.0000) ────────────────────────────
q_3    ── H ── rx(1.0000) ── H ── rx(1.0000) ── H ── rx(1.0000)

@minestarks minestarks added bug Something isn't working needs triage unitary-hack unitaryHACK 2024 bounty issues and removed needs triage labels May 3, 2024
@tcNickolas tcNickolas added good first issue Good for newcomers and removed unitary-hack unitaryHACK 2024 bounty issues labels May 9, 2024
@Pulkit1822
Copy link

@minestarks @tcNickolas , I would like to work on this issue. Can please tell me how critical is the issue of long gate names affecting the readability of ASCII art circuits in the Q# framework?

@minestarks
Copy link
Member Author

@Pulkit1822 apologies for missing your post. It's been a while so hopefully you're still interested -- I'd say this is a fairly low-priority bug, and with no-one that I know currently working on it, it's probably a good first issue to experiment with.

@ggridin
Copy link
Contributor

ggridin commented Jul 30, 2024

@minestarks
Could you describe a bit more about typical development loop here?
As far as I understand, if ...\pipsrc*.rs files are modified:

  1. Rust sources compiled using Cargo from pip directory but full build is "python ./build --pip"
  2. Rust tests are in ...\pip\src\displayable_output\test.rs (and part of Cargo build)
  3. Python tests are in ...\pip\tests\ and ...\pip\test-integration (and part of full build)
    Is there any shortcut for python only build?
  4. Local python package installed as "pip install <path to ...\pip folder>"

Any other hints?

@Morcifer
Copy link
Contributor

Morcifer commented Jan 19, 2025

@ggridin, are you still working on this issue?

I took a look at the code, and this seems to be known behavior, as can be seen circuit_tests::rotation_gate, which tests the circuit line created with a Rx(pi/2) and says in a comment "The wire isn't visible here since the gate label is longer than the static column width, but we can live with it."

I can think of several ways to resolve this issue:

  1. Change the static column width from its current 7 to be long enough to fit even the longest gates. In the current code I believe it's something like 11 (e.g. rxx(1.0000), because the precision for these rotation gates is hard-coded to 4 right now).
  2. Shorten the precision of the longest gates such that they fit the current static column of 7. For the example in this issue it means rx(1.0000) turns into rx(1.0), but for gates like rxx it probably means rxx(1.), which isn't ideal.
  3. Change the design of the circuits such that instead of static, they per-calculate the maximum gate length and build the circuit based on that length.

Option 3 is probably the only future-proof solution, but is also a fair amount of work. So I personally believe a combination of 1 & 2 is the best approach for now.

Specifically:

  • Put the static column width on 9.
  • Shorten the precision to fit 9 characters (meaning 2 instead of 4, i.e. rx(1.00) and rxx(1.00)).
  • See if we can add a generic test that makes sure that the gates will never have a length that's longer than the static column width. That way, in the future, if someone changes a fmt method somewhere, the test will inform that developer that they broke the circuit art.

Do you have opinions on this? If we go with 1 & 2 I can probably make this change myself. If we want to go with 3, someone who's more experienced with this library should probably pick it up, or at least design it.

@ggridin
Copy link
Contributor

ggridin commented Jan 21, 2025

@Morcifer
No, I am not working on the issue now. Feel free to pick it up.

@Morcifer
Copy link
Contributor

I ended up implementing the future-proof way, because it was possible to do with only one large change relative to the previous logic (no need for a complete re-design).

@Pulkit1822
Copy link

Pulkit1822 commented Jan 26, 2025

@Pulkit1822 apologies for missing your post. It's been a while so hopefully you're still interested -- I'd say this is a fairly low-priority bug, and with no-one that I know currently working on it, it's probably a good first issue to experiment with.

@minestarks I've actually been working on a solution and have already raised a PR for it. I made a couple of key changes to improve the rendering of long gate names in ASCII art circuits, specifically adjusting column widths to accommodate label length and ensuring proper alignment of columns and circuit wires. I'd love to hear any suggestions you may have to further improve the fix. Please feel free to review and let me know if there's anything I can do to enhance it. Thank you again for your time.

@minestarks
Copy link
Member Author

Thanks for the PRs - sadly we can only take one, I'll review the PRs first-come first-served and see if one looks more viable than the other.

Also, missed this message by @ggridin over the summer apparently. Fair question - I'll plan to update some READMEs in the repo to make the dev loop a bit more explicit

@minestarks Could you describe a bit more about typical development loop here? As far as I understand, if ...\pipsrc*.rs files are modified:

  1. Rust sources compiled using Cargo from pip directory but full build is "python ./build --pip"
  2. Rust tests are in ...\pip\src\displayable_output\test.rs (and part of Cargo build)
  3. Python tests are in ...\pip\tests\ and ...\pip\test-integration (and part of full build)
    Is there any shortcut for python only build?
  4. Local python package installed as "pip install <path to ...\pip folder>"

Any other hints?

@Morcifer
Copy link
Contributor

I'm really sorry about the confusion - I didn't notice it was two different people posting the questions, so I accidentally only tagged one when asking if I could pick it up. I'll make sure to pay closer attention next time!

github-merge-queue bot pushed a commit that referenced this issue Jan 29, 2025
#2126)

See issue #1476.

I tried to keep the logic as close as possible to the original, because
I wanted to keep it easy to review it in relation to the old logic.

The main change here is that instead of having the `ObjectsByColumn` of
every Row be a `FxHashMap<usize, String>` which contains the
7-width-wide strings, it's now a `FxHashMap<usize, CircuitObject>`,
where `CircuitObject` is an enum that supports every circuit object
except for than blanks and wires (because those are inserted later on,
on-the-go).

Using this enum allows us to calculate the required
`ColumnWidthsByColumn` (a `FxHashMap<usize, usize>`) right before
calling `row.fmt` - so we know what width the `CircuitObjects` need to
be when converted into strings.

In the example from the ticket, this means that this circuit:
```
use q3 = Qubit();

H(q3);
Rx(1.0, q3);
H(q3);
Rx(1.0, q3);
H(q3);
Rx(1.0, q3);
```

Which used to give:
```
q_0    ── H ─────────── ● ──── M ────────────────
                        │      ╘═════════════════
q_1    ── H ──── X ──── X ───────────────────────
q_2     rx(1.0000)  rx(1.0000) ────────────────────────────
q_3    ── H ── rx(1.0000) ── H ── rx(1.0000) ── H ── rx(1.0000)
```

Now gives
```
q_0    ────── H ─────────────────────── ● ──────── M ────────────────────────────
                                        │          ╘═════════════════════════════
q_1    ────── H ──────────── X ──────── X ───────────────────────────────────────
q_2    ─ rx(1.0000) ─── rx(1.0000) ──────────────────────────────────────────────
q_3    ────── H ─────── rx(1.0000) ──── H ─── rx(1.0000) ──── H ─── rx(1.0000) ──
```


Apart from this desired change for long gates, the only notable
difference is that, where ket-zero (`|0〉`) used to show as a width-5
circuit object (`─ |0〉 ─`, it now shows as a width-7 circuit object (`──
|0〉 ──`) - you can see the difference in every test that used it.
idavis pushed a commit that referenced this issue Feb 2, 2025
#2126)

See issue #1476.

I tried to keep the logic as close as possible to the original, because
I wanted to keep it easy to review it in relation to the old logic.

The main change here is that instead of having the `ObjectsByColumn` of
every Row be a `FxHashMap<usize, String>` which contains the
7-width-wide strings, it's now a `FxHashMap<usize, CircuitObject>`,
where `CircuitObject` is an enum that supports every circuit object
except for than blanks and wires (because those are inserted later on,
on-the-go).

Using this enum allows us to calculate the required
`ColumnWidthsByColumn` (a `FxHashMap<usize, usize>`) right before
calling `row.fmt` - so we know what width the `CircuitObjects` need to
be when converted into strings.

In the example from the ticket, this means that this circuit:
```
use q3 = Qubit();

H(q3);
Rx(1.0, q3);
H(q3);
Rx(1.0, q3);
H(q3);
Rx(1.0, q3);
```

Which used to give:
```
q_0    ── H ─────────── ● ──── M ────────────────
                        │      ╘═════════════════
q_1    ── H ──── X ──── X ───────────────────────
q_2     rx(1.0000)  rx(1.0000) ────────────────────────────
q_3    ── H ── rx(1.0000) ── H ── rx(1.0000) ── H ── rx(1.0000)
```

Now gives
```
q_0    ────── H ─────────────────────── ● ──────── M ────────────────────────────
                                        │          ╘═════════════════════════════
q_1    ────── H ──────────── X ──────── X ───────────────────────────────────────
q_2    ─ rx(1.0000) ─── rx(1.0000) ──────────────────────────────────────────────
q_3    ────── H ─────── rx(1.0000) ──── H ─── rx(1.0000) ──── H ─── rx(1.0000) ──
```


Apart from this desired change for long gates, the only notable
difference is that, where ket-zero (`|0〉`) used to show as a width-5
circuit object (`─ |0〉 ─`, it now shows as a width-7 circuit object (`──
|0〉 ──`) - you can see the difference in every test that used it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants