fix: support physical qubits in QASM3 to QIR conversion#290
Conversation
qasm3_to_qir raised a bare AssertionError (empty message) for programs addressing physical qubits, e.g. "h $0;". These are valid OpenQASM 3 and are what Qiskit emits for backend-transpiled circuits, but they unroll to plain Identifier nodes rather than IndexedIdentifier, which _get_op_bits assumed. Physical qubits now lower to the QIR qubit of the same index ($3 is qubit 3). The entry point declares enough qubits to cover the highest index used: pyqasm reports num_qubits == 0 for these programs, and a program touching only $7 still needs 8 qubits for $7 to be a valid QIR qubit id. Full-barrier detection has the same problem -- it summed declared register sizes, which is 0 with no registers, so every barrier looked like an unsupported subset barrier. Unsupported operands and target-less measurements now raise Qasm3ConversionError with a message rather than an empty AssertionError. Requires pyqasm >= 1.0.4, the first release that preserves physical qubits in reset statements (qBraid/pyqasm#325).
WalkthroughQASM3-to-QIR conversion now supports physical qubit identifiers, sizes entry points for their highest index, preserves reset mappings, validates unsupported operands and missing measurement targets, updates the ChangesPhysical qubit conversion
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 |
| include "stdgates.inc"; | ||
| bit[3] meas; | ||
| rz(pi / 2) $0; | ||
| sx $0; |
There was a problem hiding this comment.
should add the check for the single qubit op as well in the final qasm
| include "stdgates.inc"; | ||
| bit[1] meas; | ||
| h $2; | ||
| reset $2; |
There was a problem hiding this comment.
add check for reset as well
| touching only "$7" still needs 8 qubits for "$7" to be a valid QIR qubit id. | ||
| """ | ||
| finder = _HighestPhysicalQubit() | ||
| for statement in module.qasm_program.unrolled_ast.statements: |
There was a problem hiding this comment.
This will mean that we are going over the complete AST just for qubit count calculation which seems wasteful. Is it possible to derive this count from the qasm module object? Or keep a property max_physical_qubit_addr in the qasm module object? Should eliminate the need for a recalculation here
| _PHYSICAL_QUBIT_RE = re.compile(r"^\$(\d+)$") | ||
|
|
||
|
|
||
| def _physical_qubit_index(node: Any) -> Optional[int]: |
There was a problem hiding this comment.
As mentioned in the comment below, better to move the parsing semantics to pyqasm
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Thanks for catching this @ryanhill1 ! The logic seems correct but I think we would be better off keeping the attribute denoting the largest physical qubit in pyqasm.
It might be possible that physical qubit addressing related logic is needed in qBraid SDK or other dependencies so it is better to make sure that pyqasm remains the single place for semantic processing
…iving it pyqasm already registers physical qubits during unrolling and folds them into num_qubits (indices are absolute hardware addresses, so a program touching only "$7" reports 8 qubits). Walking the unrolled AST to recompute the highest index duplicated semantic processing that belongs in pyqasm, so drop _HighestPhysicalQubit / _required_qubits and read qasm3_module.num_qubits. Also extend the physical-qubit tests: assert the single-qubit ops in the Qiskit-style transpiled program (rz, and the h-s-h that "sx" decomposes to, since QIR has no native sx) and assert the reset call in the reset test.
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Thanks for the changes @ryanhill1 ! I'll wait for pyqasm 1.0.4 to be published before approval and merge
![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 `@qbraid_qir/qasm3/visitor.py`:
- Around line 236-241: Remove the redundant operation.target validation block
from the surrounding visitor logic, since _visit_measurement is the sole handler
for QuantumMeasurementStatement and already performs this check. Leave the
validation and error behavior in _visit_measurement unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cdf9cd44-e467-464f-91d6-b23229b28707
📒 Files selected for processing (4)
CHANGELOG.mdpyproject.tomlqbraid_qir/qasm3/visitor.pytests/qasm3_qir/converter/test_physical_qubits.py
| if operation.target is None: | ||
| raise_qasm3_error( | ||
| "Measurement result must be assigned to a classical bit, " | ||
| "e.g. 'c[0] = measure q[0];'", | ||
| span=operation.span, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove duplicate measurement validation.
This exact validation block was also added to _visit_measurement at lines 321-326. Since _visit_measurement is the only caller that processes QuantumMeasurementStatement, this check is redundant here and can be safely removed.
♻️ Proposed refactor
- if operation.target is None:
- raise_qasm3_error(
- "Measurement result must be assigned to a classical bit, "
- "e.g. 'c[0] = measure q[0];'",
- span=operation.span,
- )
bit_list = [operation.measure.qubit] if qubits else [operation.target]📝 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.
| if operation.target is None: | |
| raise_qasm3_error( | |
| "Measurement result must be assigned to a classical bit, " | |
| "e.g. 'c[0] = measure q[0];'", | |
| span=operation.span, | |
| ) | |
| bit_list = [operation.measure.qubit] if qubits else [operation.target] |
🤖 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 `@qbraid_qir/qasm3/visitor.py` around lines 236 - 241, Remove the redundant
operation.target validation block from the surrounding visitor logic, since
_visit_measurement is the sole handler for QuantumMeasurementStatement and
already performs this check. Leave the validation and error behavior in
_visit_measurement unchanged.
|
@ryanhill1 I've released |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
6c0394c to
d7ac111
Compare
|
@TheGupta2012 Heads up on the CI failures here — they are not caused by any change in this PR. All 325 tests pass, including the new physical-qubit coverage; the only two failures are They're dependency drift: cuda-q 0.15 now emits opaque-pointer QIR (QIR 2.0), which the I've split the fix into its own PR so this one stays scoped to the physical-qubit feature: #292. Once #292 lands on |
Co-authored-by: Harshit Gupta <harshit.11235@gmail.com>
2 participants
Summary of changes
qasm3_to_qirraised a bareAssertionErrorwith an empty message for any program addressing physical qubits, e.g.h $0;:Physical qubits are valid OpenQASM 3, and are exactly what Qiskit emits when a circuit is transpiled against a backend (
qasm3.dumps(transpile(circuit, backend))). pyqasm parses, validates, and unrolls them fine — but they survive unrolling as plainIdentifiernodes rather thanIndexedIdentifier, which_get_op_bitsassumed of every operand.The fix
$3is qubit 3 — for gates, barriers, measurements, and reset.num_qubits == 0after unrolling these programs, so the visitor now derives the count itself. Physical indices are absolute hardware addresses, so a program touching only$7still needs 8 qubits for$7to be a valid QIR qubit id._barrier_applicablesummed declared register sizes, which is0when there are no registers, so every barrier on a physical-qubit program looked like an unsupported "subset barrier" and raisedNotImplementedError.measure q;) now raiseQasm3ConversionErrorwith an actual message.Impact
This was found via 52 production job failures on qBraid's QIR simulator, where the empty exception message surfaced to users as an opaque "Internal server error". All 52 of those programs convert correctly with this change.
Tests
Six new tests in
tests/qasm3_qir/converter/test_physical_qubits.py, all failing before the fix:$3/$7keep their indices and the entry point declares 8 qubitsmeasure q;with no target raisesQasm3ConversionErrorQasm3ConversionErrorFull suite green against both pyqir versions CI covers (0.11.x and 0.12+): 325 passed, 7 skipped. pylint 10/10, black clean.
Summary by CodeRabbit
Bug Fixes
Dependency Updates