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?
The "__PYQASM_QUBITS__" literal was hardcoded across visitor.py, transformer.py
and pulse/utils.py. Move it to elements.py as INTERNAL_QUBIT_REGISTER, next to an
is_internal_qubit_register() helper that becomes the single place the register is
recognized (exact name, or name followed by an index or slice).
Applying the helper to the measurement handler fixes the same lookalike-prefix bug
already fixed for reset: startswith("__PYQASM_QUBITS__") also matched user registers
such as "__PYQASM_QUBITS__foo", short-circuiting them out of unrolling so that
"c = measure __PYQASM_QUBITS__foo;" was emitted verbatim instead of expanded per
qubit. Regression test added.
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