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 |
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Verdict
Approve with non-blocking comments (carried as prose here; this review is posted as a comment, not as a GitHub approval). The fix is correct and well-scoped. The central safety question — whether dropping QuantumPhase can ever discard an observable phase — was tested adversarially, and the answer is no, for a reason that holds structurally rather than by luck: _visit_phase_operation (src/pyqasm/visitor.py:1430-1445) rewrites a controlled phase into a p gate and clears .modifiers, so every QuantumPhase reaching _drop_global_phase is a genuine global phase. The two Medium findings below are worth addressing before merge; neither blocks.
Findings
| # | Type | Severity | Summary |
|---|---|---|---|
| 1 | Implementation | Medium | A conditional whose body is entirely global phase is emitted as a bodiless if, silently, where main raised |
| 2 | Maintenance | Medium | The conditional-body gphase diagnostic had its message edited and its only test deleted |
| 3 | Maintenance | Low | The drop is isinstance-only; correctness rests on an invariant held in visitor.py and undocumented at the drop site |
| 4 | Implementation | Low | A test comment claims the output is "a loadable QASM 2 program"; an external QASM 2 parser rejects it |
How this was verified
Every claim below comes from executing code in a worktree at 1246c656, compared against a second worktree at origin/main (7c31308).
Global phase is genuinely global — the load-bearing check. Three independent lines of evidence:
ctrl @ rzz(0.3)andctrl @ rxx(0.3), and theirnegctrl @forms, are rejected upstream —ValidationError: Unsupported controlled QASM operation: rzz with 1 controls— on bothmainand this branch. Wrappingrzzin a user-defined gate and controlling that wrapper is rejected identically.- Controlling a gate that does carry a phase reaches
_visit_phase_operationwith a non-emptyctrls, which converts it into apgate. Forgate myg(t) a { gphase(t); x a; }applied underctrl @, zeroQuantumPhasenodes survive and the emitted body ish; rx(0.3); h; cx— the phase became a real rotation rather than a dropped node. - Numerically, that emitted circuit matches
Operator(UnitaryGate(exp(i*theta)*X).control(1))under.equiv(). The repository's owntests/qasm3/test_gates.py:768 test_ctrl_gphase_eq_palready pins the samectrl @ gphase == pidentity.
Re-filter matrix, for rzz and rxx, five sequences each: remove_idle_qubits()→validate(); remove_idle_qubits() twice; remove_idle_qubits()→reverse_qubit_order(); reverse_qubit_order()→remove_idle_qubits(); unroll()→remove_measurements()→validate(). All 10 fail on main with Statement of type <class 'openqasm3.ast.QuantumPhase'> not supported in QASM 2.0. All 10 pass here, and none of the outputs contain gphase.
Emitted output under an external parser. qiskit.qasm2.loads(..., custom_instructions=LEGACY_CUSTOM_INSTRUCTIONS) accepts the unrolled rzz and rxx programs; it rejects main's output. Both decompositions do drop a nonzero phase — gphase(-0.15) for rzz(0.3) and gphase(-0.35) for rxx(0.7), i.e. -theta/2 in each case. Operator(...).equiv() confirms the emitted QASM 2 matches qiskit's RZZGate(0.3) / RXXGate(0.7) and matches pyqasm's own QASM 3 unrolling of the same source, up to global phase in every pairing — exactly the intended difference and nothing more.
Rejection of user-written gphase still holds in both positions: at top level via the whitelist in _filter_statements, and inside a conditional via the retained diagnostic. Both were confirmed under validate() and under unroll().
Containers. _drop_global_phase descends into if_block/else_block only, and that is sufficient rather than merely untested: _filter_statements rejects both Box and Pragma for a QASM 2 module (Statement of type <class 'openqasm3.ast.Box'> not supported in QASM 2.0), so the #pragma braket verbatim box path added in #341 cannot reach a Qasm2Module. BranchingStatement is the only whitelisted container, and the unroller expands loops.
No aliasing consequence follows from mutating stmt.if_block in place: unrolling the same module object twice is byte-identical, and matches a freshly loaded module.
Suite, lint, CI. Full pytest: 719 passed here against 714 on main, with the same two pre-existing tests/cli/test_cli_commands.py failures on both, which are environmental and unrelated. black --check is clean. All 26 CI checks on the PR report success.
Not verified
isort and pylint are not present in the available environment, so black was the only formatter run locally; CI covers the remainder.
Next steps
- Merge-order conflict with #338. Open PR #338 ("fix: emit OpenQASM 2 syntax for classical conditionals") rewrites this same file with +364 lines, adding
_filter_branch,Qasm2Printerand_flatten_branch. Whichever of the two merges second will conflict insrc/pyqasm/modules/qasm2.py. Finding 1 interacts with it directly, since #338 introduces the printer that would have to render a branch emptied by the phase drop. Sequencing the two deliberately will cost less than resolving that conflict blind. - Pre-existing and out of scope, but adjacent enough to deserve an issue:
inv @ rzz(0.3) q[0], q[1];raisesAttributeError: 'QuantumPhase' object has no attribute 'name'on bothmainand this branch.
| if isinstance(stmt, qasm3_ast.BranchingStatement): | ||
| stmt.if_block = self._drop_global_phase(stmt.if_block) | ||
| stmt.else_block = self._drop_global_phase(stmt.else_block) | ||
| filtered.append(stmt) |
There was a problem hiding this comment.
Type: Implementation | Severity: Medium
Rationale: The drop can empty a conditional body, and nothing then removes the now-bodiless branch. A gate whose body is nothing but a phase reaches this:
gate ph(t) a { gphase(t); }
if(c==1) ph(0.3) q[1];
On this branch that unrolls to if (c[0] == true) { followed by a bare closing brace — an if with no qop, which QASM 2 has no form for. remove_idle_qubits() and reverse_qubit_order() both preserve it. main was wrong here too, but loudly: it emitted gphase(0.3) q[1] inside the branch and raised ValidationError on reload. This PR turns a rejection into silently malformed output, which is the worse failure mode.
Narrow reachability, granted — rzz/rxx never hit it, since their decompositions carry real gates alongside the phase. But #345 already established the precedent for exactly this shape: a box left empty by a removal is dropped, "since pyqasm rejects a box with no statements".
Change requested: Drop the branch when both blocks come back empty. Guarding on if_block alone would be wrong if an else_block ever survives.
| if isinstance(stmt, qasm3_ast.BranchingStatement): | |
| stmt.if_block = self._drop_global_phase(stmt.if_block) | |
| stmt.else_block = self._drop_global_phase(stmt.else_block) | |
| filtered.append(stmt) | |
| if isinstance(stmt, qasm3_ast.BranchingStatement): | |
| stmt.if_block = self._drop_global_phase(stmt.if_block) | |
| stmt.else_block = self._drop_global_phase(stmt.else_block) | |
| if not stmt.if_block and not stmt.else_block: | |
| # the body was nothing but global phase, and QASM 2 has no | |
| # form for a conditional without a qop | |
| continue | |
| filtered.append(stmt) |
| if isinstance(stmt, qasm3_ast.QuantumPhase): | ||
| continue |
There was a problem hiding this comment.
Type: Maintenance | Severity: Low
Rationale: The drop keys on the node type alone. It is correct today, and that was confirmed rather than assumed: _visit_phase_operation in visitor.py clears .modifiers and rewrites any controlled phase into a p gate, so nothing carrying a control modifier ever arrives here. That invariant lives in another module, though, and nothing at this site records the dependency. Should it ever weaken, a controlled phase — a relative phase, and fully observable — would vanish with no error and no visible signal. Silent physical incorrectness is expensive to find later.
Change requested: Assert the invariant instead of relying on it. raise_qasm3_error is already imported at line 26.
| if isinstance(stmt, qasm3_ast.QuantumPhase): | |
| continue | |
| if isinstance(stmt, qasm3_ast.QuantumPhase): | |
| # a controlled phase is relative, not global, and is observable; | |
| # the visitor rewrites those to 'p' gates, so none should reach here | |
| if stmt.modifiers: | |
| raise_qasm3_error( | |
| "Modified global phase cannot be dropped for a QASM 2 target", | |
| error_node=stmt, | |
| span=stmt.span, | |
| ) | |
| continue |
| 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)""" | ||
| module = loads(QASM2_PREAMBLE + "if(m==1) rzz(0.3) q[0], q[1];\n") | ||
| module.unroll() | ||
| module.reverse_qubit_order() | ||
| with pytest.raises(ValidationError, match="Global phase is not representable in QASM 2.0"): | ||
| module.remove_idle_qubits() | ||
| module.remove_idle_qubits() |
There was a problem hiding this comment.
Type: Maintenance | Severity: Medium
Rationale: This PR edits the wording of the conditional-body gphase diagnostic (qasm2.py:101-106) and, in the same change, removes the only test that exercised it. grep -rn "not representable in QASM 2.0" tests/ returns nothing on this branch; on main it matched the pytest.raises(...) this test replaced.
The new test_user_written_gphase_rejected in test_operations.py does not close the gap: a top-level gphase is caught by the whitelist in _filter_statements and produces a different message (Statement of type <class 'openqasm3.ast.QuantumPhase'> not supported in QASM 2.0). The conditional-body path is still live — if(m==1) gphase(0.3); was confirmed to raise the trimmed message under both validate() and unroll() — but it is now untested, immediately after its text was changed.
Change requested: Keep this test as written, and add one alongside it for the rejection path, so the branch this PR reworded stays pinned:
def test_conditional_user_gphase_rejected():
"""A gphase the user wrote in a conditional body is still rejected (issue #351)."""
module = loads(QASM2_PREAMBLE + "if(m==1) gphase(0.3);\n")
with pytest.raises(ValidationError, match="Global phase is not representable in QASM 2.0"):
module.validate()| # the unrolled output must be a loadable QASM 2 program | ||
| loads(unrolled).validate() |
There was a problem hiding this comment.
Type: Implementation | Severity: Low
Rationale: The comment claims more than the assertion establishes. loads(...).validate() proves pyqasm re-reads its own output; it does not prove the output is QASM 2. It is not: qiskit.qasm2.loads rejects this exact string with QASM2ParseError: needed '==', but instead saw [, because the conditional prints as if (c[0] == true) { ... } — QASM 3 syntax. Emitting gphase was one reason this output would not load, but not the only one.
That remaining half is #338's territory rather than this PR's, so no code change is needed here. A future reader taking the comment at face value would conclude the conditional path is fully fixed, though, and it is not.
Change requested: Narrow the comment to what is actually asserted — for example # the unrolled output must still re-load in pyqasm — or keep the stronger claim and reference #338 as the remaining blocker.
3 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