fix: preserve physical qubits in reset statements#325
Conversation
_visit_reset rewrote a physical qubit operand to the internal pulse register unconditionally, so "reset $2;" unrolled to "reset __PYQASM_QUBITS__[2];". That names a register the program never declares, so the unrolled output did not round-trip through dumps()/loads(), and the qubit was never registered -- a program whose only operation was "reset $3;" reported num_qubits == 0. Gate and measurement operands already branch on the OpenPulse grammar flag and keep "$n" as-is for plain QASM programs. Reset now does the same, applying the rename only for OpenPulse programs, where the pulse visitor expects it.
|
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 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❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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
🧹 Nitpick comments (1)
src/pyqasm/visitor.py (1)
711-714: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an OpenPulse regression case.
The added tests cover plain-QASM preservation, counting, and reloadability, but not the OpenPulse branch that rewrites
$nto__PYQASM_QUBITS__[n]. Add a test asserting that behavior remains valid.🤖 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 711 - 714, Add an OpenPulse regression test covering the branch in the visitor that handles _openpulse_grammar_declared and rewrites a $n qubit reference to __PYQASM_QUBITS__[n]. Assert the transformed name is correct and the resulting program remains valid, while preserving the existing plain-QASM test coverage.
🤖 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 698-705: Add an Args entry to _resolve_unindexed_reset_qubit’s
docstring documenting the statement parameter as the QuantumReset being
resolved, while preserving the existing Returns documentation.
- Line 722: Update the internal-register check in the visitor method containing
the __PYQASM_QUBITS__ prefix test so it matches only the generated indexed
register form, not user identifiers with the same prefix; preserve
_get_op_bits() validation and unrolling for all other operands.
In `@tests/qasm3/test_reset.py`:
- Around line 123-148: Update all three newly added test functions in
tests/qasm3/test_reset.py to declare an explicit -> None return annotation and
add a “Returns: None” section to each test’s docstring, including
test_reset_physical_qubit_preserves_identifier and the two adjacent tests.
---
Nitpick comments:
In `@src/pyqasm/visitor.py`:
- Around line 711-714: Add an OpenPulse regression test covering the branch in
the visitor that handles _openpulse_grammar_declared and rewrites a $n qubit
reference to __PYQASM_QUBITS__[n]. Assert the transformed name is correct and
the resulting program remains valid, while preserving the existing plain-QASM
test coverage.
🪄 Autofix (Beta)
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
Run ID: e2d5b345-2d11-42b2-9175-c4a5cb130a4e
📒 Files selected for processing (3)
CHANGELOG.mdsrc/pyqasm/visitor.pytests/qasm3/test_reset.py
Raised in review. The reset path treated any identifier starting with
"__PYQASM_QUBITS__" as the internal pulse register, which also swallowed a
user register that merely starts with that name:
qubit[2] __PYQASM_QUBITS__foo;
reset __PYQASM_QUBITS__foo; // silently reset nothing
The statement was short-circuited out of unrolling and emitted verbatim,
where a normally-named register expands to one reset per qubit.
Match the exact name, or the name followed by an index or a slice, which are
the forms the transformer actually generates ("__PYQASM_QUBITS__",
"__PYQASM_QUBITS__[2]", "__PYQASM_QUBITS__[0:2]"). Matching only the indexed
form, as first suggested, would silently drop the bare and slice forms.
Also documents the helper's parameter.
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Reserved register that physical qubits are consolidated onto for OpenPulse programs. | ||
| _INTERNAL_QUBIT_REGISTER = "__PYQASM_QUBITS__" |
There was a problem hiding this comment.
This seems like a good refactoring opportunity. Can you use this across the file so that this convention is embedded properly across the code base?
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Looks good mostly @ryanhill1 , can you just trickle the _INTERNAL_QUBIT_REGISTER across the file?
3 participants
Summary of changes
reseton a physical qubit rewrote the operand to the internal pulse register:reset $2;unrolled toreset __PYQASM_QUBITS__[2];.Two consequences:
__PYQASM_QUBITS__is a register the program never declares, so the unrolled program does not round-trip throughdumps()→loads()._register_physical_qubit, so a program whose only operation isreset $3;reportsnum_qubits == 0.Gate and measurement operands already branch on
_openpulse_grammar_declaredand keep$nas-is for plain QASM programs (# Plain QASM program: keep the physical qubit identifier as-is.). Reset was applying the OpenPulse rename unconditionally. It now follows the same rule: keep$nand register the qubit for plain QASM, rename only for OpenPulse programs, where the pulse visitor expects it.Physical qubits are valid OpenQASM 3 and are what Qiskit emits when a circuit is transpiled against a backend (
qasm3.dumps(transpile(circuit, backend))), so this shape shows up in real user programs.How it was found
Downstream in
qbraid-qir, whose QIR visitor could not lower the__PYQASM_QUBITS__[2]identifier. Surfaced from 52 production job failures on qBraid's QIR simulator.Tests
Three new tests in
tests/qasm3/test_reset.py, each failing before the fix:test_reset_physical_qubit_preserves_identifier— unrolled output keepsreset $2;test_reset_physical_qubit_is_counted—reset $3;alone givesnum_qubits == 4test_reset_physical_qubit_unrolled_output_is_reloadable— the unrolled program reloads and validatesFull suite green with the CI extras (
test,cli,pulse): 674 passed, 3 skipped (baseline 671 + these 3). pylint 10/10, mypy and black clean.Summary by CodeRabbit
Bug Fixes
resetoperations in plain OpenQASM so operands remain unchanged.Tests