fix: drop unroll-emitted global phase for QASM 2 targets - #358
Conversation
rzz/rxx decompose to a body containing a QuantumPhase node, which Qasm2Module emitted as gphase(...) — syntax OpenQASM 2 does not have, so the unrolled output was not a loadable QASM 2 program. Global phase is unobservable, so Qasm2Module.accept() now strips QuantumPhase statements (descending into conditional bodies) from the unrolled AST. User-written gphase in QASM 2 source is still rejected. Fixes #351
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:
📝 WalkthroughWalkthroughQASM 2 unrolling now removes generated global phases from top-level and conditional statements. User-written ChangesQASM 2 global-phase handling
Estimated code review effort: 3 (Moderate) | ~20 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 |
|
|
@coderabbitai review |
|
|
@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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pyqasm/modules/qasm2.py (1)
151-176: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd required type annotations and complete docstrings.
_drop_global_phasehas no parameter or return annotation. Its docstring has noArgsorReturnssection.acceptalso has an untypedvisitorparameter and no documented return value. Add compatible annotations and document theNonereturn values.As per coding guidelines,
**/*.pyrequires type annotations for all functions and methods, and docstrings that explain purpose, parameters, and return values.🤖 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/modules/qasm2.py` around lines 151 - 176, Add compatible type annotations to _drop_global_phase, including its statements parameter and list return type, and document the parameter and returned filtered statements in its docstring. Annotate accept’s visitor parameter with the appropriate visitor type and its return type as None, then add a Returns section documenting that it returns None.Source: Coding guidelines
🧹 Nitpick comments (1)
tests/qasm2/test_conditional_body.py (1)
67-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep coverage for authored conditional
gphase.This test replaces the conditional global-phase rejection test.
test_user_written_gphase_rejectedcovers only a top-level statement. Add a companion test that verifiesif(m==1) gphase(0.3);raisesValidationError, becauseQasm2Module._filter_branch_body()owns this separate validation path.🤖 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 `@tests/qasm2/test_conditional_body.py` around lines 67 - 73, Add a companion test near test_conditional_rzz_survives_refiltering that loads an authored conditional gphase statement, such as if(m==1) gphase(0.3);, and asserts module.unroll() raises ValidationError. Keep the existing refiltering test unchanged and target the conditional validation path implemented by Qasm2Module._filter_branch_body().
🤖 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 `@CHANGELOG.md`:
- Line 29: Update the changelog entry describing the `rzz`/`rxx` fix to
explicitly state that OpenQASM 2 unrolling emitted invalid `gphase(...)` output,
while preserving the existing details about dropping unroll-emitted phases and
rejecting user-written `gphase`.
In `@tests/qasm2/test_conditional_body.py`:
- Around line 67-69: Add the return type annotation -> None to
test_conditional_rzz_survives_refiltering, and extend its docstring with a
Returns section documenting that the test returns None.
In `@tests/qasm2/test_operations.py`:
- Around line 70-72: Update all affected test functions in this file, including
test_rzz_unrolls_without_gphase and the additional referenced tests, to declare
-> None and extend each docstring with a Returns section documenting that the
function returns None.
---
Outside diff comments:
In `@src/pyqasm/modules/qasm2.py`:
- Around line 151-176: Add compatible type annotations to _drop_global_phase,
including its statements parameter and list return type, and document the
parameter and returned filtered statements in its docstring. Annotate accept’s
visitor parameter with the appropriate visitor type and its return type as None,
then add a Returns section documenting that it returns None.
---
Nitpick comments:
In `@tests/qasm2/test_conditional_body.py`:
- Around line 67-73: Add a companion test near
test_conditional_rzz_survives_refiltering that loads an authored conditional
gphase statement, such as if(m==1) gphase(0.3);, and asserts module.unroll()
raises ValidationError. Keep the existing refiltering test unchanged and target
the conditional validation path implemented by
Qasm2Module._filter_branch_body().
🪄 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: 9b6c4e00-49f0-49df-ad43-a03e36765e9d
📒 Files selected for processing (4)
CHANGELOG.mdsrc/pyqasm/modules/qasm2.pytests/qasm2/test_conditional_body.pytests/qasm2/test_operations.py
| ### Removed | ||
|
|
||
| ### Fixed | ||
| - Fixed `rzz`/`rxx` in an OpenQASM 2 program unrolling to a `gphase(...)` statement, which QASM 2 has no syntax for, so the output was not a loadable QASM 2 program. Global phase is unobservable, so unroll-emitted phases are now dropped for a QASM 2 target; a user-written `gphase` is still rejected. ([#351](https://github.com/qBraid/pyqasm/issues/351)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the failed unrolling behavior.
The phrase “rzz/rxx in an OpenQASM 2 program unrolling to” is unclear. State that OpenQASM 2 unrolling of rzz and rxx emitted invalid gphase(...) output.
🤖 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 `@CHANGELOG.md` at line 29, Update the changelog entry describing the
`rzz`/`rxx` fix to explicitly state that OpenQASM 2 unrolling emitted invalid
`gphase(...)` output, while preserving the existing details about dropping
unroll-emitted phases and rejecting user-written `gphase`.
| def test_conditional_rzz_survives_refiltering(): | ||
| """Test that transformations which re-filter an already unrolled body no longer | ||
| trip over the rzz global phase: it is dropped for a QASM 2 target (issue #351)""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the test annotation and return documentation.
Add -> None. Add a Returns section that documents the None return value.
As per coding guidelines, **/*.py requires type annotations for all functions and docstrings that explain return values.
🤖 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 `@tests/qasm2/test_conditional_body.py` around lines 67 - 69, Add the return
type annotation -> None to test_conditional_rzz_survives_refiltering, and extend
its docstring with a Returns section documenting that the test returns None.
Source: Coding guidelines
| def test_rzz_unrolls_without_gphase(): | ||
| """Test that the global phase from the rzz decomposition is dropped for a QASM 2 | ||
| target, which has no global-phase syntax (issue #351)""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add test function annotations and return documentation.
Add -> None to each test function. Add a Returns section that documents the None return value.
As per coding guidelines, **/*.py requires type annotations for all functions and docstrings that explain return values.
Also applies to: 98-100, 126-127, 146-148, 163-165
🤖 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 `@tests/qasm2/test_operations.py` around lines 70 - 72, Update all affected
test functions in this file, including test_rzz_unrolls_without_gphase and the
additional referenced tests, to declare -> None and extend each docstring with a
Returns section documenting that the function returns None.
Source: Coding guidelines
|
Applied the changelog wording fix. Skipping the |
2 participants
Fixes #351
rzz/rxxdecompose to a body containing aQuantumPhasenode, whichQasm2Moduleemitted asgphase(...)— syntax OpenQASM 2 does not have, so unrolling a legal qelib1 gate produced output no QASM 2 parser accepts. The same node also broke re-filtering afterreverse_qubit_order()/remove_idle_qubits().Global phase is unobservable, so
Qasm2Module.accept()now stripsQuantumPhasestatements (descending into conditional bodies) from the unrolled AST. A user-writtengphasein QASM 2 source is still rejected, and the #339 interim diagnostic is retained for that case with its message trimmed.Summary by CodeRabbit
Bug Fixes
rzzandrxxoperations.gphasestatements remain unsupported and are rejected.Tests