Multi-target gate circuit art improvement #2185

Morcifer · 2025-02-15T12:35:02Z

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:

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.
c141799 A cleanup based on this comment. It makes the code significantly better, in my opinion.
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.
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 · 2025-02-15T12:45:26Z

compiler/qsc_circuit/src/circuit.rs

+                let next_qubit = qubit + 1;
+
+                // Check if the next qubit is also in this operation.
+                if operation.targets.iter().any(|t| t.q_id == next_qubit) {


Technically, in terms of pure complexity, it would make more sense to make a hashsets out of these ids, inside the loop of line 336, and use it in this line (with contains(&next_qubit)).

However, given the overhead of creating hashsets, and moreover, the relatively slow performance of hashsets in Rust (even FxHashSet, from my experience), I'm almost sure that this loop is faster for gates with a small number of target qubits.

But if it's actually common for these gates to have a large number of targets, I can easily replace this bit of code and use a FxHashSet here, just let me know.

Agree with the reasoning. It would have to be a custom intrinsic in Q# that takes a very large number of qubits, and I just don't expect to see that in practice.

minestarks

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

minestarks · 2025-02-18T19:33:02Z

compiler/qsc_circuit/src/circuit.rs

@@ -325,6 +330,26 @@ impl Display for Circuit {
        // to row in the diagram
        let mut register_to_row = FxHashMap::default();

+        // Keep track of which qubits have the qubit after them in the same multi-qubit operation.
+        let mut consequtive_qubits = FxHashSet::default();


Minor typo (consecutive).

But actually consider renaming this. I spent some time scratching my head over how this could possibly work, because I thought the set literally contained some consecutive qubit IDs (e.g. 2, 3, 4, 5) 😄

How about qubits_with_gap_row_below? Not a pretty name, but at least it's less likely to send the reader on a wild goose chase.

minestarks · 2025-02-18T20:35:33Z

compiler/qsc_circuit/src/circuit.rs

        end_column: usize,
    ) -> std::fmt::Result {
        // Temporary string so we can trim whitespace at the end
+        let default_renderer = ColumnRenderer::default();


The comment above this line got pushed up - it's supposed to refer to this line below this.

However I think you can just use unwrap_or_default below instead of declaring a default here. It's not going to incur an extra allocation or anything like that (since the struct is just an int anyway)

unwrap_or_default doesn't work, because column_renderers.get(&column).unwrap() is a &ColumnRenderer, while the default part of unwrap_or_default returns an actual ColumnRenderer.

And doing something like .unwrap_or(&ColumnRenderer::default()) also doesn't work because of temporary value dropped while borrowed, because I can't reference something made inside this.

Rust is fighting against me here. Can you think of another way to do it that won't end up moving (or needing to clone) the renderer we get from column_renderers?

Oh, interesting. I would just add #[derive(Clone, Copy)] on the struct to stop "fighting" rust. It's really not a big deal cloning this object since it's simply a usize.

The solution to the ColumnWidthsByColumn naming question (in the latest commit) ended up solving this, as well as simplifying this code quite a lot (especially with clippy's suggestion of using enumerate rather than keep the for loop in this method).

Let me know what you think.

minestarks · 2025-02-18T20:36:39Z

compiler/qsc_circuit/src/circuit.rs

@@ -96,7 +96,7 @@ impl Config {
 }

 type ObjectsByColumn = FxHashMap<usize, CircuitObject>;
-type ColumnWidthsByColumn = FxHashMap<usize, usize>;
+type ColumnWidthsByColumn = FxHashMap<usize, ColumnRenderer>;


I like how you organized this. I just think we can simplify the names.

Suggested change

type ColumnWidthsByColumn = FxHashMap<usize, ColumnRenderer>;

type FullColumns = FxHashMap<usize, Column>;

That's what the new type is... it's just the column! No need to overthink it imo.

And now we can think of the hashmap as containing only the columns that have objects in them, so I suggested a name that reflects that.

I'm not 100% sure I agree with this:

My thinking is that the "ByColumn" postfix is consistent with ObjectsByColumn in the row above, because both are a hashmap with the key being the column index.

And the ColumnRenderer itself is a struct that takes circuit objects and turns them into ASCII art, so calling it a Column sounds a bit confusing to me.

Not to mention how, if you agree with my point about ByColumn but don't agree with this point, then we'll alias this hashmap as ColumnByColumn, and that's just silly. 😄

Having said that, I will of course rename both of these if you still think your suggested names are better.

Actually, I realize now that I can resolve most of this comment and the other open one pretty easily, it's an oversight on my side - this alias isn't necessary, because we don't actually need a hashmap for this. Let me try it out...

You'll still have to decide between Column and ColumnRenderer, though.

Okay, short answer: Totally agree with you on the bit where you say it would be inconsistent with ObjectsByColumn map right above this - I hadn't noticed that. Consistency trumps all, so let's stick with your version. Maybe consider ColumnWidth instead of ColumnRenderer.

Long answer: humor me for a second while I try to explain what I originally had in mind. Naming discussions tend to get pretty philosophical.

"column index": it's just a number and that's what the original hashmap index usize refers to.

"column": that's the struct that describes the "physical" column in the diagram, so to speak. It contains attributes of the column, such as its width.

Personally I'm biased against struct names like "Something-Doer" because I prefer structs to have names that describe what the thing is, not what it does.

I guess then the natural conclusion would have been to rename these to ObjectsByColumnIndex and ColumnsByColumnIndex ... and then rename all the other variables that refer to columns below to column_index ... and I mean... that is getting into silly territory.

Yeah, that works - please look at the latest commit. Basically, since the column renderers are made by iterating over 0..end_column, they can just be put in a vector. That means there's no need for this hashmap at all.

I can still rename the ColumnRenderer to Column, though then the enumeration in lines 192 and 200 will be over (column_index, column) instead of the current (column, renderer). Not sure what's clearer in this case.

minestarks · 2025-02-18T20:48:38Z

compiler/qsc_circuit/src/circuit.rs

+                let next_qubit = qubit + 1;
+
+                // Check if the next qubit is also in this operation.
+                if operation.targets.iter().any(|t| t.q_id == next_qubit) {


Agree with the reasoning. It would have to be a custom intrinsic in Q# that takes a very large number of qubits, and I just don't expect to see that in practice.

minestarks · 2025-02-18T20:53:48Z

compiler/qsc_circuit/src/circuit.rs

@@ -335,7 +360,15 @@ impl Display for Circuit {

            register_to_row.insert((q.id, None), rows.len() - 1);

-            for i in 0..q.num_children {
+            // If this qubit has no children, but it is in a multi-qubit operation with
+            // the next qubit, we add one classical row for clarity.


Suggested change

// the next qubit, we add one classical row for clarity.

// the next qubit, we add an empty row to make room for the vertical connector.

// We can just use a classical wire type for this row since the wire won't actually be rendered.

I now see how you're exploiting the "classical" wire type to create an empty row. I think that's fine (no need to overengineer it and add a 3rd wire type) but it's subtle enough that I think it warrants an explanatory comment.

minestarks · 2025-02-18T20:59:30Z

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

Morcifer added 5 commits February 1, 2025 17:00

How this code should have looked in the first place

a400655

This actually is nicer

c141799

Minor simplification with new and default

4012ee8

Inefficient in terms of complexity, but works as expected

99910c5

Both efficient and correct (probably)

cfa7382

Morcifer commented Feb 15, 2025

View reviewed changes

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

Morcifer requested review from idavis, minestarks and swernli as code owners February 15, 2025 12:47

minestarks requested changes Feb 18, 2025

View reviewed changes

minestarks requested a review from ScottCarda-MS February 18, 2025 20:58

Morcifer added 2 commits February 19, 2025 19:32

Rework, non-argumentative part

aa6ae78

Rework part 2 - 1.5 comments, one stone.

1d1e039

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-target gate circuit art improvement #2185

Multi-target gate circuit art improvement #2185

Morcifer commented Feb 15, 2025 •

edited

Loading

Morcifer Feb 15, 2025 •

edited

Loading

minestarks Feb 18, 2025

minestarks left a comment

minestarks Feb 18, 2025

minestarks Feb 18, 2025

Morcifer Feb 19, 2025 •

edited

Loading

minestarks Feb 19, 2025

Morcifer Feb 19, 2025 •

edited

Loading

minestarks Feb 18, 2025

Morcifer Feb 19, 2025 •

edited

Loading

Morcifer Feb 19, 2025

minestarks Feb 19, 2025

Morcifer Feb 19, 2025 •

edited

Loading

minestarks Feb 18, 2025

minestarks Feb 18, 2025

minestarks commented Feb 18, 2025

	type ColumnWidthsByColumn = FxHashMap<usize, ColumnRenderer>;
	type FullColumns = FxHashMap<usize, Column>;

	// the next qubit, we add one classical row for clarity.
	// the next qubit, we add an empty row to make room for the vertical connector.
	// We can just use a classical wire type for this row since the wire won't actually be rendered.

Multi-target gate circuit art improvement #2185

Are you sure you want to change the base?

Multi-target gate circuit art improvement #2185

Conversation

Morcifer commented Feb 15, 2025 • edited Loading

Morcifer Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minestarks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morcifer Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morcifer Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morcifer Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morcifer Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minestarks commented Feb 18, 2025

Morcifer commented Feb 15, 2025 •

edited

Loading

Morcifer Feb 15, 2025 •

edited

Loading

Morcifer Feb 19, 2025 •

edited

Loading

Morcifer Feb 19, 2025 •

edited

Loading

Morcifer Feb 19, 2025 •

edited

Loading

Morcifer Feb 19, 2025 •

edited

Loading