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
2 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