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

Multi-target gate circuit art improvement #2185

Merged
merged 8 commits into from
Feb 24, 2025

Conversation

Morcifer
Copy link
Contributor

@Morcifer Morcifer commented Feb 15, 2025

This PR has a proposed fix for issue #2184, but it mainly includes refactoring of the circuit-art code based on comments from PR #2126.

The only real functionality change in this code is that multi-target gates get extra lines that behave in the same way as measurement lines, in order to better convey the qubits involved.

For example, a circuit like this one:

use q0 = Qubit();
use q1 = Qubit();
Rxx(1.0, q0, q1);

use q2 = Qubit();
use q3 = Qubit();
Rxx(1.0, q2, q3);

instead of rendering as

q_0    ─ rxx(1.0000) ─
q_1    ─ rxx(1.0000) ─
q_2    ─ rxx(1.0000) ─
q_3    ─ rxx(1.0000) ─

will instead render as

q_0    ─ rxx(1.0000) ─
              ┆
q_1    ─ rxx(1.0000) ─
q_2    ─ rxx(1.0000) ─
              ┆
q_3    ─ rxx(1.0000) ─

This is very similar to what we get if we measure the first qubit of each of these two-qubit gate, i.e.

q_0    ─ rxx(1.0000) ─── M ──
              ┆          ╘═══
q_1    ─ rxx(1.0000) ────────
q_2    ─ rxx(1.0000) ─── M ──
              ┆          ╘═══
q_3    ─ rxx(1.0000) ────────

On top of this change in rendering, the first few commits in this PR are actually just code refactoring. As such, it's probably best to review this PR commit by commit, for two reason - a. it's going to be much easier to follow, and b. any of these commits can be reverted if you think they make the code more complicated instead of making it easier to read.

The commits are as follows:

  1. a400655 A rework of PR Long gate in ASCII art circuits - lengthen column width when necessary #2126, which, to be honest, is how I think I should have implemented it in the first place.
  2. c141799 A cleanup based on this comment. It makes the code significantly better, in my opinion.
  3. 4012ee8 A small simplification to have the ColumnRenderer have a new method which is in charge of making sure that the column width is an odd number, and also a Default trait implementation for the case where we don't need a specific width.
  4. cfa7382 This is the actual enhancement - it makes sure there is a classical line (same as what you get from a measurement) if there are two consecutive qubits for the same operation. If you think there is a better solution to make the rendering clearer, let me know. But of the ones I could think of, I found this one to be the least disruptive one (to both the existing code, and the existing circuits).

@Morcifer Morcifer marked this pull request as ready for review February 15, 2025 12:47
Copy link
Member

@minestarks minestarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix and additional test cases. Just a few requests for readability.

@minestarks
Copy link
Member

Heads up to @ScottCarda-MS who I think has been working near this code.

@Morcifer Morcifer requested a review from minestarks February 22, 2025 08:52
@minestarks minestarks added this pull request to the merge queue Feb 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2025
@swernli swernli added this pull request to the merge queue Feb 24, 2025
Merged via the queue into microsoft:main with commit 8fbe81d Feb 24, 2025
18 checks passed
@minestarks
Copy link
Member

Thanks @Morcifer !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants