Render zero-operand instructions (e.g. GlobalPhaseGate) in circuit drawers#16453
Render zero-operand instructions (e.g. GlobalPhaseGate) in circuit drawers#16453KevesDev wants to merge 9 commits into
Conversation
`dag.layers()` only ever visits nodes reachable by walking the DAG's wires, so an instruction with no qargs and no cargs (such as a manually appended GlobalPhaseGate) is never seen by `_LayerSpooler` and gets silently dropped before any drawer even sees it. `_get_layered_instructions` now separately walks `dag.op_nodes()` for zero-operand nodes and re-inserts each one as its own dedicated layer, positioned immediately after the layer of the last preceding instruction in original circuit order. DAGOpNode identity/equality is not stable across separate `dag.layers()`/`dag.op_nodes()` calls, so matching is done via the (qargs, cargs) of each instruction instead, which is stable. Part of Qiskit#9962.
Now that zero-operand instructions survive layering, draw each one as a box spanning every qubit wire, with no per-wire operand index since there's no real operand to index (per the resolution proposed in Qiskit#9962). A single-qubit circuit degenerates to the same plain box used for any other one-qubit gate. Also fixes a latent bug in `_set_multibox`'s single-bit path: it always forwarded `top_connect` (default `None`) straight into the box constructor, overriding that class's own default ("-") with `None` and breaking `.center()` calls on it later. No existing caller hit this combination, but the new zero-operand path does whenever the circuit being drawn has only one qubit.
Once the layering fix let zero-operand nodes through, the matplotlib drawer crashed in `_multiqubit_gate` (min() on an empty q_xy list) instead of its previous behavior of silently dropping the instruction. Drawing it correctly is a net improvement either way, but a crash is strictly worse than the bug it was meant to fix. `_get_coords` now treats a zero-operand node's qubit span as every qubit in the circuit, and `_multiqubit_gate` skips the per-wire operand-index annotations (and their associated box padding) for such nodes, matching the text drawer's behavior.
Same crash-regression pattern as matplotlib: once zero-operand nodes reach _build_latex_array with an empty wire_list, _build_multi_gate called min()/max() on it and raised. _build_latex_array now treats a zero-operand node's wire_list as every qubit in the circuit, and _build_multi_gate gained a skip_wire_labels option that omits the per-wire operand-index subscripts on the \multigate/\ghost macros, matching the text and matplotlib drawers. Reference .tex files for the new tests were generated by running the tests once and reviewing the auto-created output, per the existing assertEqualToReference convention in this test module.
reproduce.py was useful for local development (see commit 92da65f), but a loose script at the repo root isn't idiomatic for this project -- the actual reproduction is now covered by the committed unit tests. Removing it before opening the PR.
Rebuilding the Rust extension after rebasing onto upstream/main surfaced a real gap: a zero-qarg, zero-carg *directive* (e.g. a classical Store on a Var, which has _directive=True) was also being caught by the broad "zero operand" filter and inserted into its own layer, which crashed the text drawer's barrier-handling branch (min() on an empty qargs) and would have introduced a visibly empty column even once that crash was fixed. Directives have never had any rendering for the zero-operand case and shouldn't gain one here -- only real zero-operand gates (like GlobalPhaseGate) should. _insert_zero_operand_layers now excludes directives outright, and the text drawer's directive branch no-ops instead of crashing if a zero-qarg directive reaches it by some other path (e.g. justify="none", which bypasses this layering fix entirely). Confirmed via git stash that latex.py has a similar, pre-existing, unrelated crash for zero-operand directives in _build_barrier -- not fixed here, as it occurs identically without any of this branch's changes and is out of scope for issue Qiskit#9962.
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
|
Hi @enavarro51 — this is my first contribution to Qiskit. I've fixed #9962 (zero-operand instructions like |
|
Pushed a follow-up commit (1870092) before any review: while double-checking the directive-exclusion fix against a clean build of unmodified |
The "exclude directives" commit (4118f4d) only fixed _utils.py's *insertion* path (_insert_zero_operand_layers), but missed that the *pruning* filter at the end of _get_layered_instructions has the same problem from a different angle. A classical Store on a Var is already naturally reachable via dag.layers() (Vars have their own DAG wire-tracking, unlike qubits), so it reaches _LayerSpooler's output on its own, independent of the insertion logic. On unmodified upstream, that final filter line (`any(q in qubits for q in node.qargs)`) coincidentally drops it, since its qargs are empty -- the same mechanism that was, until this PR, also incorrectly dropping real zero-operand gates. My fix to that line to stop dropping real zero-operand gates also stopped dropping Store, which then reached each drawer's directive-handling code (none of which has ever supported a zero-qarg directive) and crashed. Verified precisely with a from-scratch comparison: built upstream/main (cd146f5) in an isolated git worktree with none of this branch's commits, confirmed the Store/Var repro does NOT crash there, then confirmed it DID crash on this branch before this commit. The earlier claim in 4118f4d's message that this was a pre-existing, unrelated bug was wrong -- it was introduced by this PR. No upstream issue needed; fixing it here instead. Extracted the shared _is_zero_operand_gate(node) predicate (zero qargs, zero cargs, not a directive) so the insertion and pruning paths can't silently diverge on this distinction again.
1870092 to
e1d04e8
Compare
What does this PR do?
Zero-operand instructions — most commonly a stand-alone
GlobalPhaseGateappended viacircuit.append(GlobalPhaseGate(angle))— were being silently dropped by every circuit drawer.circ.draw('text'),circ.draw('mpl'), andcirc.draw('latex_source')would all just omit the instruction entirely, with no error and no visual trace that it had ever been there.This PR makes all three drawers render such instructions as a box spanning every qubit wire, with no per-wire operand index (since there's no specific operand to index), positioned in the circuit's original instruction order — the rendering behavior suggested in the issue itself.
Before:
(both
GlobalPhaseGate(pi)andGlobalPhaseGate(5*pi)calls are simply missing — no trace of them anywhere in the output)After:
Why was this PR needed?
The root cause is upstream of all three drawers, in shared layering code:
_get_layered_instructions(qiskit/visualization/circuit/_utils.py) builds its layers fromdag.layers(), which only ever visits DAG nodes reachable by walking the DAG's wires. An instruction with no qargs and no cargs has no wires, sodag.layers()never visits it, and it's dropped before any drawer-specific rendering code ever runs.Fix:
_get_layered_instructionsnow separately walksdag.op_nodes()for zero-operand nodes (excluding directives, see below) and re-inserts each one into its own dedicated layer, positioned immediately after the layer of the last real instruction that precedes it in original circuit order.DAGOpNodeidentity/equality isn't stable across separatedag.layers()/dag.op_nodes()calls (confirmed empirically: the same logical node gets a different_node_id, and even a differentid(node.op), depending on which traversal returns it), so matching is done via each instruction's(qargs, cargs)instead, which is stable.text.py,matplotlib.py, andlatex.pyeach extend their existing multi-qubit box-drawing code (set_qu_multibox,_multiqubit_gate,_build_multi_gate) with askip_wire_labels/equivalent option, rather than adding new, parallel drawing functions that duplicate that logic.Storeon aVar, which also has no qargs/cargs) are explicitly excluded from the layering fix — they've never had any rendering for the zero-operand case, and inserting one into its own layer would just produce a visibly empty column. Discovered this while testing againstVar-based control-flow circuits;text.py's barrier-handling branch also had a latent crash (min()on an emptyqargs) for this case, fixed defensively in case a zero-qarg directive reaches it via another path (e.g.justify="none", which bypasses the layering fix entirely).text.py's_set_multibox: its single-bit path always forwardedtop_connect(defaultNone) straight into the box constructor, silently overriding that class's own sensible default and breaking a later.center()call. No existing caller hit this combination before; the new zero-operand path does, on any single-qubit circuit.I also checked a previous attempt at this issue (#12922, stalled in draft). Its
_LayerSpoolerpatch walkeddag.topological_op_nodes()and inserted every zero-operand node at the very start of the circuit, since topological sort has no ordering for nodes with no dependency edges — multipleGlobalPhaseGates would all land in column 0 regardless of where they were declared. This PR instead places each one in true original-circuit-order. That PR also had no implemented tests and an author-acknowledged "hack" for the matplotlib portion.What are the relevant issue numbers?
Fixes #9962
Screenshots / Recordings
Text drawer output is included inline above. For the matplotlib drawer, the same circuit renders as a box spanning all 5 qubits for each
GlobalPhaseGate, with no per-wire numbers — happy to attach a rendered PNG in review if it's helpful. I did not add a matplotlib snapshot/reference-image test totest/visual/mpl/circuit/test_circuit_matplotlib_drawer.py, since generating a correctly-matched reference image requires this project's mybinder snapshot-testing notebook (per CONTRIBUTING.md) to match CI's exact rendering environment, which I can't do from a local clone — flagging this explicitly as a follow-up rather than skipping it silently. I did add a non-snapshot regression test (test_mpl_zero_operand_gate_does_not_raise) covering the same code path.Does this PR meet the acceptance criteria?
test_utils.py,test_circuit_text_drawer.py,test_circuit_drawer.py, andtest_circuit_latex.py(the latter using the project's existingassertEqualToReferencetext-diff mechanism, with committed reference.texfiles).blackandruffboth pass clean on every modified file.Also added a reno release note (
releasenotes/notes/fix-zero-operand-drawers-5b92cd1d081d2aeb.yaml).AI/LLM disclosure