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 |
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 deep-copy their qubit operands via a shared_fresh_qubitshelper, andDecomposerdeep-copies the operands of each rule-emitted gate. Every decomposition composes these constructors, so no emitted statement can share a node with another statement or with the source operation.With fresh nodes,
reverse_qubit_order()'s existing marker logic is correct as-is, and this branch (without #332's guard) independently resolves the #331 repro. #332's guard remains useful belt-and-braces and merges cleanly alongside this.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()output.🤖 Generated with Claude Code