fix: emit distinct x gates with fresh operands in negctrl expansion - #357
fix: emit distinct x gates with fresh operands in negctrl expansion#357ryanhill1 wants to merge 3 commits into
Conversation
The negctrl modifier expansion placed the same QuantumGate object at both the leading and trailing position of the emitted statement list, so in-place transformations reached the same operand nodes twice and reverse_qubit_order() raised KeyError on any unrolled negctrl gate. Fixes #350
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 |
|
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:
📝 WalkthroughWalkthroughThe negative-control expansion now creates separate leading and trailing X-gate statements with fresh operand nodes. Tests cover single- and multi-qubit controls and verify successful qubit-order reversal. ChangesNegative-control expansion
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pyqasm/visitor.py`:
- Around line 1614-1618: Update the nested _neg_x_gates function with a concise
docstring describing that it creates X gates for the negative-control qubits and
returns the resulting list of qasm3_ast.QuantumGate objects; note that it takes
no parameters.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 30436573-a75a-409f-8e45-04afa54dfe69
📒 Files selected for processing (3)
CHANGELOG.mdsrc/pyqasm/visitor.pytests/qasm3/test_transformations.py
| def _neg_x_gates() -> list[qasm3_ast.QuantumGate]: | ||
| return [ | ||
| qasm3_ast.QuantumGate([], qasm3_ast.Identifier("x"), [], fresh_qubits(ctrl)) | ||
| for ctrl in negctrls | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a docstring to _neg_x_gates.
The new nested function has no docstring. Add a concise description of its purpose and return value.
As per coding guidelines, every Python function must have a docstring explaining its purpose, parameters, and return values.
Proposed fix
def _neg_x_gates() -> list[qasm3_ast.QuantumGate]:
+ """Create X gates for the negative controls.
+
+ Returns:
+ list[qasm3_ast.QuantumGate]: The generated X gates.
+ """
return [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _neg_x_gates() -> list[qasm3_ast.QuantumGate]: | |
| return [ | |
| qasm3_ast.QuantumGate([], qasm3_ast.Identifier("x"), [], fresh_qubits(ctrl)) | |
| for ctrl in negctrls | |
| ] | |
| def _neg_x_gates() -> list[qasm3_ast.QuantumGate]: | |
| """Create X gates for the negative controls. | |
| Returns: | |
| list[qasm3_ast.QuantumGate]: The generated X gates. | |
| """ | |
| return [ | |
| qasm3_ast.QuantumGate([], qasm3_ast.Identifier("x"), [], fresh_qubits(ctrl)) | |
| for ctrl in negctrls | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pyqasm/visitor.py` around lines 1614 - 1618, Update the nested
_neg_x_gates function with a concise docstring describing that it creates X
gates for the negative-control qubits and returns the resulting list of
qasm3_ast.QuantumGate objects; note that it takes no parameters.
Source: Coding guidelines
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Verdict: approve with minor comments
The fix is correct, minimal, and consistent with the precedent #335 set for decomposition-emitted statements. It resolves issue #350 and, as a side effect, a second crash the description does not mention. Nothing here blocks merge. Four comments below: two Medium about documentation and coverage of that second crash, one Low about test breadth.
No security findings. The change adds no input handling, no I/O and no dependency, and fresh_qubits() was already public within pyqasm.maps.gates. No public API or shared contract moves, so the blast radius stays inside this repository.
Findings
- Medium — the fix also repairs
unroll(consolidate_qubits=True)crashing withKeyError: '__PYQASM_QUBITS__'on anynegctrl @gate, which the CHANGELOG entry does not mention. - Medium — regression coverage exercises
reverse_qubit_order()only.remove_idle_qubits()is named in #350 andconsolidate_qubits=Truecrashed outright onmain; neither is asserted. - Low — the
test_unroll_emits_fresh_operand_nodesmatrix gains two plainnegctrlrows, while six other operand shapes the aliasing reached stay uncovered.
How it was tested
Two worktrees were compared: PR head 624ec7c4 and origin/main at 7c31308. Every claim below comes from running code.
Issue #350 reproduces on main and is fixed here. For one, two and three negative controls, reverse_qubit_order() after unroll() raises KeyError: -5 on main and returns correct output on this branch.
Object identity. Every statement, operand node and index node in the unrolled AST was compared by id(). On main, the leading and trailing x are one object with one shared operand node and one shared index node per negative control. On this branch, zero shared objects across every case tried: one, two and three negative controls, ctrl @ negctrl @, negctrl @ ctrl @, pow(2) and pow(3) outer modifiers, a user-defined gate, physical qubits ($0), a broadcast register control (negctrl @ x q, r;), a discrete-set control (q[{0,1}]), negctrl inside a gate body invoked twice, inside a for body, inside an if body, and inside a box body.
Semantics are unchanged. The unrolled text this branch emits is byte-identical to main on all 14 cases main can unroll, so the x … x sandwich keeps its order. Independently, qiskit.quantum_info.Operator was compared for negctrl @ <gate> against its pyqasm expansion across 15 programs (qiskit 2.1.2, qiskit-qasm3-import 0.6.0). All 15 match exactly, not merely up to global phase. That set includes the reverse_qubit_order() output, checked against a hand-reversed reference program, and the inv @ negctrl @ and negctrl @ inv @ combinations on rz — s cannot serve here, since controlled sdg is unsupported on both worktrees for unrelated reasons.
Suite and lint. pytest tests on this branch: 717 passed, 2 failed, 4 skipped. On main: 714 passed, 2 failed, 4 skipped. The two failures are the same pre-existing tests/cli/test_cli_commands.py cases on both sides, so this PR adds no failure and adds 3 tests. black --check and isort --check-only are clean. pylint on src/pyqasm/visitor.py and on tests/qasm3/test_transformations.py produces no message this branch does not already produce on main; the changed region is clean. mypy src reports no error in visitor.py, and the # type: ignore on line 1621 was checked by removal and is still required. All 26 checks on gh pr checks 357 pass. The headers lint target was not run, though the PR adds no file.
Next steps
- Extend the CHANGELOG entry to name the
consolidate_qubits=Truecrash — a suggestion block is inline. - Add the two missing pass assertions to the new regression test.
- Optionally widen the
test_unroll_emits_fresh_operand_nodesmatrix.
| ### Removed | ||
|
|
||
| ### Fixed | ||
| - Fixed the `negctrl @` expansion emitting the same `x` `QuantumGate` object at both the leading and trailing position, so in-place transformations mutated its operands twice — crashing `reverse_qubit_order()` (`KeyError`) on any unrolled `negctrl` gate. The two `x` gates are now distinct statements with fresh operand nodes. ([#350](https://github.com/qBraid/pyqasm/issues/350)) |
There was a problem hiding this comment.
Type: Maintenance
Severity: Medium
Rationale: This entry credits the fix with repairing reverse_qubit_order() only, but the same aliasing broke a second user-visible path that this PR also fixes. On origin/main, unroll(consolidate_qubits=True) raises KeyError: '__PYQASM_QUBITS__' for any negctrl @ gate:
qubit[3] q;
negctrl @ x q[0], q[1];
# origin/main: KeyError: '__PYQASM_QUBITS__'
# (transformer.py:480, _get_pyqasm_device_qubit_index)
# 624ec7c4: x __PYQASM_QUBITS__[0];
# cx __PYQASM_QUBITS__[0], __PYQASM_QUBITS__[1];
# x __PYQASM_QUBITS__[0];
Same root cause: consolidate_qubit_registers rewrote the shared x operand once, then met the already-renamed node a second time and looked __PYQASM_QUBITS__ up in global_qreg_size_map. A reader scanning the changelog to see whether their crash is fixed will not find it under the current wording, so the release notes understate the change.
Change Requested: Name the second crash in the entry.
| - Fixed the `negctrl @` expansion emitting the same `x` `QuantumGate` object at both the leading and trailing position, so in-place transformations mutated its operands twice — crashing `reverse_qubit_order()` (`KeyError`) on any unrolled `negctrl` gate. The two `x` gates are now distinct statements with fresh operand nodes. ([#350](https://github.com/qBraid/pyqasm/issues/350)) | |
| - Fixed the `negctrl @` expansion emitting the same `x` `QuantumGate` object at both the leading and trailing position, so in-place transformations mutated its operands twice — crashing `reverse_qubit_order()` (`KeyError`) and `unroll(consolidate_qubits=True)` (`KeyError: '__PYQASM_QUBITS__'`) on any unrolled `negctrl` gate. The two `x` gates are now distinct statements with fresh operand nodes. ([#350](https://github.com/qBraid/pyqasm/issues/350)) |
| _assert_no_shared_operand_nodes(module) | ||
|
|
||
|
|
||
| def test_reverse_qubit_order_negctrl(): |
There was a problem hiding this comment.
Type: Implementation
Severity: Medium
Rationale: This test covers one of the three consumers the aliasing broke. Issue #350 names remove_idle_qubits() explicitly, noting it survives only because of the visited_node_ids guard in Modules._remap_qubits — an incidental guard rather than a deliberate contract, so nothing stops a later refactor from dropping it and silently reintroducing the crash. And unroll(consolidate_qubits=True) did not merely survive on main; it raised KeyError: '__PYQASM_QUBITS__' outright. Both paths are fixed by this change and neither is asserted, which leaves the fix resting on a single pass.
Change Requested: Add two short cases alongside this one, so all three passes the aliasing reached are pinned:
remove_idle_qubits()on the unrollednegctrl @ x q[0], q[1];— expectqubit[2] q; x q[0]; cx q[0], q[1]; x q[0];unroll(consolidate_qubits=True)on the same program — expectqubit[3] __PYQASM_QUBITS__; x __PYQASM_QUBITS__[0]; cx __PYQASM_QUBITS__[0], __PYQASM_QUBITS__[1]; x __PYQASM_QUBITS__[0];
Both expected outputs were confirmed against this commit.
| "ecr q[0], q[1];", | ||
| "inv @ crz(0.5) q[1], q[2];", | ||
| "negctrl @ x q[0], q[1];", | ||
| "negctrl(2) @ x q[0], q[1], q[2];", |
There was a problem hiding this comment.
Type: Maintenance
Severity: Low
Rationale: These two rows cover the plain negctrl shape only. Testing on origin/main shows the aliasing also reached six further operand shapes that this change fixes but that no test pins: a broadcast register control (negctrl @ x q, r;), a discrete-set control (negctrl @ x q[{0,1}], q[2];), negctrl inside a gate body invoked twice, negctrl inside a for body, pow(n) @ negctrl @, and ctrl @ negctrl @. Each takes a different _copy_qubit branch or repeats the expansion, so each is a distinct way for the pattern to regress. This is the fourth fix for one bug class (#331, #333, #335, #350), which argues for making this matrix the durable guard rather than adding a bespoke test per incident.
Change Requested: Optional. Consider adding two or three of the above as parametrize rows — one line each, no new helper needed.
3 participants
Fixes #350
The
negctrl @expansion placed the sameQuantumGateobject at both the leading and trailing position of the emitted statement list. In-place transformations therefore reached the same operand nodes twice:reverse_qubit_order()raisedKeyErroron any unrollednegctrlgate, andremove_idle_qubits()survived only via thevisited_node_idsguard.The two
xgates are now built as distinct statements whose operands go throughfresh_qubits(), matching the fix #335 made for decomposition-emitted statements.Summary by CodeRabbit
Bug Fixes
xoperations.Tests
Documentation