fix: emit fresh operand nodes from gate decompositions - #335
Conversation
Gate decomposition functions passed the same IndexedIdentifier objects into every statement they emitted, so in-place index rewrites in remove_idle_qubits() and reverse_qubit_order() mutated a shared node once per referencing statement, raising KeyError whenever the remap was not the identity. Statement constructors in maps/gates.py and Decomposer now deep-copy their qubit operands so every emitted statement owns its nodes. Fixes #333
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| (e.g. ``remove_idle_qubits``, ``reverse_qubit_order``) would mutate a | ||
| shared node once per referencing statement. | ||
| """ | ||
| return [deepcopy(qubit) for qubit in qubits] |
There was a problem hiding this comment.
Does an IndexedIdentifier have nested statements as well? If not, maybe a shallow copy can work here too. The performance of deepcopy is known to be a little slow, and since this _fresh_qubits operation will be used quite frequently, it is best to figure out the optimized approach for copying
There was a problem hiding this comment.
Good catch on the cost — but a shallow copy unfortunately isn't enough here.
IndexedIdentifier has no nested statements, but it does have a nested node tree: IndexedIdentifier.name is an Identifier, and .indices is a list[list[Expression]] holding the IntegerLiteral. The transforms mutate the innermost node in place:
bit.indices[0][0].value = idx_map[old_idx] # _remap_qubits
bit.indices[0][0].value = -1 * new_reg_idx - 1 # reverse_qubit_orderso copy.copy leaves the very object that gets mutated shared:
s = copy(q)
s.indices[0][0].value = 99
q.indices[0][0].value # 99 <- still aliasedWhat does work, and is faster than both, is rebuilding the node tree by hand. At this point in the pipeline every operand is either a plain reg[int] or a bare physical-qubit Identifier, so the copy is three tiny constructor calls with no reflection:
def _copy_qubit(qubit):
if isinstance(qubit, Identifier):
fresh = Identifier(name=qubit.name)
else:
indices = qubit.indices
if not (len(indices) == 1 and isinstance(indices[0], list)
and len(indices[0]) == 1 and isinstance(indices[0][0], IntegerLiteral)):
return deepcopy(qubit) # fallback for any other operand shape
fresh = IndexedIdentifier(
name=Identifier(name=qubit.name.name),
indices=[[IntegerLiteral(value=indices[0][0].value)]],
)
fresh.span = qubit.span
return freshMicrobenchmark, 200k iterations on q[3]:
| approach | time |
|---|---|
deepcopy |
1.550 s |
copy (insufficient) |
0.182 s |
| structural rebuild | 0.110 s |
~14x faster than deepcopy, and still faster than the shallow copy it can't use. deepcopy is kept only as a fallback for operand shapes that aren't a plain reg[int] (e.g. a DiscreteSet or multi-dimensional index), so correctness doesn't depend on that assumption holding forever.
span is carried over explicitly so error reporting on emitted statements is unchanged.
Added test_fresh_qubits_shares_no_nodes covering the plain, physical-qubit, and fallback paths.
| qubits=qubits, | ||
| # copy so the emitted gates do not share operand nodes with each | ||
| # other or with the source statement (see #333) | ||
| qubits=[deepcopy(qubit) for qubit in qubits], |
There was a problem hiding this comment.
Why not use _fresh_qubits here too?
There was a problem hiding this comment.
Done — good call. Renamed the helper to fresh_qubits (dropping the leading underscore now that it crosses module boundaries) and _get_decomposed_gates now calls it:
qubits=fresh_qubits(*qubits),decomposer.py no longer imports deepcopy at all.
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Thanks for flagging this @ryanhill1 , can you see if copy works here as well? If yes, best to use that instead of deepcopy.
…erand-nodes # Conflicts: # CHANGELOG.md
Addresses review feedback on #335: - A shallow `copy` cannot replace `deepcopy` here: transforms rewrite the index in place as `bit.indices[0][0].value = ...`, so the nested `IntegerLiteral` must be a distinct object, which `copy` does not give. Instead, `_copy_qubit` rebuilds the (small, fully known) `reg[int]` node tree directly — ~14x faster than `deepcopy` in a microbenchmark, and faster than `copy.copy` as well — falling back to `deepcopy` only for operand shapes that are not a plain `reg[int]`. - `Decomposer._get_decomposed_gates` now uses the shared `fresh_qubits` helper (renamed from `_fresh_qubits` now that it crosses modules) instead of its own inline `deepcopy` comprehension. - Added `test_fresh_qubits_shares_no_nodes` covering the plain, physical qubit, and `deepcopy` fallback paths.
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
Estimated cost
Tip: you can also comment |
There was a problem hiding this comment.
Approved with comments — the right fix at the right level: #331 and #333 were two symptoms of one defect, and this fixes the source rather than guarding each consumer. One test gap worth closing before merge; the other notes are follow-ups.
Findings
All findings are posted inline on the relevant lines. Checklist:
- M1 —
decomposer.pyhunk is unpinned; reverting it leaves the suite green - M2 —
negctrlpath still aliases statement objects; PR body over-promises (follow-up) - L1 —
#332guard comment is now untrue and invites deletion of load-bearing code - L2 — Test helper assumes an indexed operand shape
How it was tested
- Repro:
crz(0.5) q[1],q[2]+unroll()+reverse_qubit_order()raisesKeyError: -1onmain, passes here. - Aliasing invariant: 0 shared operand/index nodes after
unroll()acrosscrz,crx,ecr,inv @ crz,ccx,c4x. - Reverting
qubits=fresh_qubits(*qubits)indecomposer.pyalone, leavinggates.pyintact, still leaves the full suite green (see M1). - Removing the
#332guard from the branch:crzfine,negctrl @ xraisesKeyError: 0, suite still green (see L1). - Instrumenting
Decomposer._get_decomposed_gatesto confirm which gates reach it:crz→ no calls,swap→ one call. - Microbenchmark reproduced (200k iterations on
q[3]):deepcopy1.499 s, shallowcopy0.168 s,_copy_qubit0.112 s. Mutating a shallow copy'sindices[0][0].valuewrites through to the original;_copy_qubit's result does not. - Perf end to end: 200
crzgates → 2402 statements in 0.046 s; thedeepcopyfallback is not on the hot path. - Suite on branch: 647 passed, 4 skipped. Branch current with
main(0 behind),MERGEABLE. - The parametrized test suggested in M1 was applied and run both ways: 3 passed with the fix; reverting
decomposer.pyfails theswapandczcases and passescrz.
Next steps
Add the swap/cz case from M1 to the rebase test — that is the one ask before merge; it is attached as an applicable suggestion. M2 and L1 want follow-up issues (and L1 a one-line comment edit), L2 is optional. Independent of the rest of the open batch, so it can land at any point in the sequence.
| return fresh | ||
|
|
||
|
|
||
| def fresh_qubits(*qubits: QubitOperand) -> list[QubitOperand]: |
There was a problem hiding this comment.
[M2] negctrl path still aliases statement objects — Implementation · Medium (pre-existing; follow-up issue, not a blocker)
Rationale: this helper delivers what it promises for decomposition-emitted statements. But the PR description says "no emitted statement can share a node with another statement or with the source operation", and the unroller has an independent aliasing source this PR does not touch, in visitor.py:
negs = [qasm3_ast.QuantumGate([], qasm3_ast.Identifier("x"), [], [ctrl]) for ctrl in negctrls]
result = negs + result + negsThat places the same QuantumGate object at two positions in the statement list, not two equal copies. Verified on this branch:
negctrl @ x q[0], q[1]; -> x q[0]; cx q[0], q[1]; x q[0];
statement object ids: [4337616448, 4336546384, 4337616448]
So reverse_qubit_order() still walks the same operand node twice and raises KeyError: -2. Identical behaviour on main (unroll() + reverse_qubit_order() on negctrl @ x fails on both), so this is not a regression from this PR — it is outside the stated scope, and no test covers it on either side.
Change requested: nothing in this PR. Open a follow-up for the negctrl path (presumably a second negs list rather than reusing the first, plus fresh_qubits on ctrl), and consider narrowing the sentence in the PR body to "decomposition no longer emits shared nodes" so the changelog does not over-promise.
There was a problem hiding this comment.
Acknowledged — follow-up filed as #350, PR body narrowed.
Reproduced on this branch: statement object ids for negctrl @ x q[0], q[1]; come back [4362757584, 4362758224, 4362757584], so positions 0 and 2 are the same object. Left out of scope here since it's pre-existing on main and lives in the modifier expansion, not decomposition.
The PR body now says "no statement emitted by a decomposition can share a node", with an explicit scope note pointing at #350.
| qubits=qubits, | ||
| # copy so the emitted gates do not share operand nodes with each | ||
| # other or with the source statement (see #333) | ||
| qubits=fresh_qubits(*qubits), |
There was a problem hiding this comment.
[L1] The #332 guard comment is now untrue — and the guard is still load-bearing — Maintenance · Low
Concerns the visited_node_ids guard in _remap_qubits, src/pyqasm/modules/base.py — not changed by this PR, hence the anchor here.
Rationale: the PR body describes that guard as "useful belt-and-braces" now that nodes are fresh. That undersells it, and the understatement is the risk — it invites a future reader to delete it as dead code. Removing the guard from this branch and re-running gives:
crz : remove_idle_qubits OK <- this PR did make the guard redundant here
negctrl @ x : remove_idle_qubits KeyError: 0 <- still the only thing holding this up
647 passed, 4 skipped <- no test would have caught it
So the guard is precisely what keeps remove_idle_qubits() working on the negctrl circuits from M2, and nothing in the suite protects it.
Change requested: update the comment above visited_node_ids to name the surviving case (statements re-inserted by the negctrl expansion), rather than "gate decompositions can reuse the same operand node" — which this PR has now made untrue. One line, but it is the difference between the next reader keeping it and removing it.
There was a problem hiding this comment.
Resolved in 6620dbe — comment rewritten to name the surviving case.
Verified the guard is still load-bearing: remove_idle_qubits() on negctrl @ x passes only because of it. The comment now reads:
# the negctrl expansion in the visitor re-inserts the same QuantumGate object before
# and after the controlled gate, so a single index node can be reached more than once;
# track visited nodes so it is remapped exactly onceCross-referenced from #350 so the two stay linked.
| check_unrolled_qasm(dumps(module), expected_qasm3_str) | ||
|
|
||
|
|
||
| def _assert_no_shared_operand_nodes(module): |
There was a problem hiding this comment.
[L2] Helper assumes an indexed operand shape — Maintenance · Low
Rationale: _assert_no_shared_operand_nodes does bit.indices[0][0] unconditionally, so it would raise on a bare-Identifier physical qubit or a DiscreteSet index rather than reporting a clean assertion failure. Not hit by the current parameters, and test_fresh_qubits_shares_no_nodes covers those shapes directly.
Change requested: none required. Worth a guard only if this helper gets reused for physical-qubit circuits later — noted so the limitation is known rather than rediscovered.
There was a problem hiding this comment.
Resolved in 6620dbe — guard added, since it was cheap.
_assert_no_shared_operand_nodes still checks operand-node identity for every bit, but skips the index check for bare Identifier operands and non-plain index shapes rather than raising on bit.indices[0][0].
| def test_rebase_emits_fresh_operand_nodes(): | ||
| """Test that rebase() never emits statements sharing operand nodes (issue #333)""" | ||
| qasm3_str = """ | ||
| OPENQASM 3.0; | ||
| include "stdgates.inc"; | ||
| qubit[3] q; | ||
| crz(0.5) q[1], q[2]; | ||
| """ | ||
| module = loads(qasm3_str).rebase(BasisSet.ROTATIONAL_CX) | ||
| _assert_no_shared_operand_nodes(module) |
There was a problem hiding this comment.
[M1] This test does not reach the decomposer.py hunk — Implementation · Medium
Rationale: test_rebase_emits_fresh_operand_nodes uses crz, which is not in DECOMPOSITION_RULES (ROTATIONAL_CX covers x, y, z, h, s, t, sx, sdg, tdg, cz, swap). Instrumenting Decomposer._get_decomposed_gates confirms the test never reaches it:
crz (the test in this PR) -> _get_decomposed_gates calls: []
swap (suggested) -> _get_decomposed_gates calls: ['swap']
crz is handled entirely by the maps/gates.py constructors, so this rebase test is really a second unroll test. Consequence: reverting qubits=fresh_qubits(*qubits) back to qubits=qubits in decomposer.py, leaving gates.py intact, still leaves the entire suite passing — 647 passed, including this test. Half the diff is currently unpinned.
The hunk is genuinely fixing something, to be clear. On main, swap q[0], q[2] rebased to CLIFFORD_T emits three cx statements sharing both index nodes:
cx [('q', 0, 4345354976), ('q', 2, 4345355792)]
cx [('q', 2, 4345355792), ('q', 0, 4345354976)]
cx [('q', 0, 4345354976), ('q', 2, 4345355792)]
4 duplicate operand nodes and 4 duplicate index nodes on main, 0 on this branch. _get_qubits_for_gate hands back references straight into statement.qubits, so every rule-emitted gate aliases the source statement. The fix is correct; it just isn't tested.
Change requested: parametrize over a gate that actually reaches _get_decomposed_gates. The suggestion below was applied and run both ways — 3 passed with the fix in place, and reverting the decomposer.py line fails the swap and cz cases while crz still passes.
| def test_rebase_emits_fresh_operand_nodes(): | |
| """Test that rebase() never emits statements sharing operand nodes (issue #333)""" | |
| qasm3_str = """ | |
| OPENQASM 3.0; | |
| include "stdgates.inc"; | |
| qubit[3] q; | |
| crz(0.5) q[1], q[2]; | |
| """ | |
| module = loads(qasm3_str).rebase(BasisSet.ROTATIONAL_CX) | |
| _assert_no_shared_operand_nodes(module) | |
| @pytest.mark.parametrize( | |
| "operation", ["crz(0.5) q[1], q[2];", "swap q[0], q[2];", "cz q[1], q[2];"] | |
| ) | |
| def test_rebase_emits_fresh_operand_nodes(operation): | |
| """Test that rebase() never emits statements sharing operand nodes (issue #333) | |
| ``swap`` and ``cz`` are in DECOMPOSITION_RULES so they exercise | |
| Decomposer._get_decomposed_gates; ``crz`` is handled by maps/gates.py instead. | |
| """ | |
| qasm3_str = f""" | |
| OPENQASM 3.0; | |
| include "stdgates.inc"; | |
| qubit[3] q; | |
| {operation} | |
| """ | |
| module = loads(qasm3_str).rebase(BasisSet.ROTATIONAL_CX) | |
| _assert_no_shared_operand_nodes(module) |
Note for whoever applies it: don't reach for
rebase(...)followed byreverse_qubit_order()as the regression test — the rebase is silently discarded before the remap runs, so that test would pass for the wrong reason.
There was a problem hiding this comment.
Resolved in 6620dbe — suggestion applied as written.
Confirmed your finding before and after: with qubits=fresh_qubits(*qubits) reverted in decomposer.py, the swap and cz cases fail and crz still passes; with the fix in place all three pass. The hunk is pinned now.
- test_rebase_emits_fresh_operand_nodes now parametrizes over swap and cz, which reach Decomposer._get_decomposed_gates; crz alone never did, so the decomposer.py hunk was unpinned (M1) - _remap_qubits guard comment now names the negctrl expansion, the case that still re-inserts the same statement object, rather than gate decompositions, which this PR made fresh (L1) - _assert_no_shared_operand_nodes skips the index check for bare Identifier and non-plain index shapes instead of raising (L2)
3 participants
Summary of changes
Closes #333. Root-cause fix for the operand-node aliasing behind #331 as well.
Gate decomposition functions in
maps/gates.pypass the same qubit operand nodes into every statement they emit (e.g.crz_gatehandsqubit1to twou3_gateand twocxexpansions), so the unrolled AST containedIndexedIdentifierobjects shared across many statements. Any transformation that rewrites qubit indices in place then mutated a shared node once per referencing statement:remove_idle_qubits()walked off its index map →KeyError(remove_idle_qubits() KeyError: unroll() emits aliased operand nodes that _remap_qubits mutates repeatedly #331, point-fixed by the visited-node guard in fix: remove_idle_qubits KeyError on shared operand nodes #332);reverse_qubit_order()'s negative-marker pass read an already-marked shared node →KeyError: -1(reverse_qubit_order() KeyError on aliased operand nodes (same root cause as #331) #333, no guard);Decomposer._get_decomposed_gateshad the same pattern, sorebase()output was aliased too.This PR fixes the class of bug at the source: the five statement constructors in
maps/gates.py(one_qubit_gate_op,one_qubit_rotation_op,two_qubit_gate_op,ccx_gate_op,global_phase_gate) now copy their qubit operands via a sharedfresh_qubitshelper, andDecomposerdoes the same for the operands of each rule-emitted gate. Every decomposition composes these constructors, so no statement emitted by a decomposition can share a node with another statement or with the source operation.Scope note: the
negctrlmodifier expansion invisitor.pyis an independent aliasing source that this PR does not touch — it re-inserts the sameQuantumGateobject before and after the controlled gate. That is pre-existing onmainand tracked separately in #350. Thevisited_node_idsguard added in #332 is what keepsremove_idle_qubits()working on those circuits, so it remains load-bearing rather than redundant.fresh_qubitsrebuilds the (small, fully known) operand node tree by hand rather than callingdeepcopy— a shallowcopyis not sufficient because transforms mutate the nestedIntegerLiteralin place. Microbenchmark over 200k iterations onq[3]:deepcopy1.550 s, shallowcopy0.182 s (and incorrect), structural rebuild 0.110 s.deepcopyis kept only as a fallback for operand shapes that are not a plainreg[int].Verified beyond the test suite: the reversed/remapped
crzcircuits are unitarily equivalent to the expected reference circuits via qiskitOperator.equiv.Tests
test_reverse_qubit_order_gate_decomposition: the reverse_qubit_order() KeyError on aliased operand nodes (same root cause as #331) #333 repro, checked against the full expected unrolled program.test_unroll_emits_fresh_operand_nodes: invariant test asserting no two quantum statements share an operand or index node afterunroll(), parametrized overcrz,crx,c4x,ecr, andinv @ crz.test_rebase_emits_fresh_operand_nodes: same invariant forrebase(), parametrized overswapandcz(which route throughDecomposer._get_decomposed_gates) andcrz(which does not).test_fresh_qubits_shares_no_nodes: covers the plainreg[int], physical-qubit, anddeepcopy-fallback paths of the helper.