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
| 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?
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.
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