fix: leave physical qubits alone when consolidating registers - #344
Conversation
Consolidation assumed every gate operand was an IndexedIdentifier and
read `qubit.name.name`, so any gate on a physical qubit (which survives
unrolling as `Identifier("$n")`) crashed with AttributeError.
Physical qubits are absolute hardware indices belonging to no declared
register, so gate operands now pass through unchanged, as measure, reset
and barrier already do.
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR. Running Argus review... 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:
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! |
![argus-eye[bot]](https://avatars.githubusercontent.com/in/2969822?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Argus · 10/10 — Argus reviewed 3 files and found no issues.
🔍 PR intent vs diff (LLM analysis)
Argus read the diff against the stated intent. This is not an execution log — reviewer still needs to test behavior.
Goal: Fix consolidate_qubit_registers so gates on physical qubits pass through unchanged instead of raising AttributeError during unroll(consolidate_qubits=True).
Stated acceptance criteria (from PR/issue — not independently verified):
- Gate operands that are not IndexedIdentifier pass through unchanged during qubit-register consolidation.
- Physical qubit operands such as $1 and $2 remain written as-is for gates when consolidate_qubits=True.
- A consolidated PYQASM_QUBITS declaration is still emitted for programs mixing declared registers with physical qubits.
✅ Intent delivered
Argus reviewed 3 files and found no issues. Code looks good.
🔢 94.3k tokens · $0.3033 total
| Stage | Tokens | Cost |
|---|---|---|
| Intent | 2.4k | $0.0013 |
| Triage | 2.1k | $0.0004 |
| Lead agent | 1.2k | $0.0036 |
| Review · bug_hunter | 18.8k | $0.0563 |
| Review · security | 18.8k | $0.0554 |
| Review · architecture | 18.5k | $0.0565 |
| Review · regression | 18.9k | $0.0568 |
| Review | 12.5k | $0.0702 |
| Acceptance | 1.2k | $0.0030 |
Contract: production/full · checked: bug_hunter, security, architecture, regression · review took 58s
Dashboard → · React 👎 to dismiss · Reply to any inline comment or use @argus-eye help to chat
There was a problem hiding this comment.
Approved with comments — the crash fix is correct, minimal, and lands in exactly the right spot. The one design question raised inline is explicitly not a blocker on this diff.
Findings
All findings are posted inline on the relevant lines. Checklist:
- M1 — Mixed declared/physical programs emit two address spaces;
num_qubitscollapses them - L1 — Unreferenced
__PYQASM_QUBITS__declaration emitted when nothing was consolidated - L2 — New tests pin current semantics without asserting
num_qubits
How it was tested
- Issue repro:
h $1;andh $0; cx $0, $1;underunroll(consolidate_qubits=True)— both succeed, output re-parses and re-validates. - Regression guard:
qubit[2] a; qubit[3] b; h a[0]; cx a[1], b[2];still consolidates toqubit[5] __PYQASM_QUBITS__;with correct offsets (a[1]→[1],b[2]→[4]). - Physical qubits inside a
box; decomposedcrz(0.5) $0, $1;→ 12 statements all on$0/$1; mixedmeasure/reset/barrier/gate on$0in one program — all round-trip. - The two mixed-address-space shapes in M1 re-run on
mainthrough themeasure/resetpaths — identical output and identical counts, confirming pre-existing #325 semantics. tests/qasm3/test_device_qubits.py: 14 passed. Full suite: 638 passed, 4 skipped (excludingtests/cli, which fails identically onmainin this sandbox — environment artifact).blackclean. Branch current withmain.- Merged worktree of #341 + #344 + #345 confirms this closes one of the two "Not fixed here" items in #341.
Next steps
Nothing blocking — merge when ready. Worth opening a follow-up issue for M1 and folding L1 into it. L2 is attached as an applicable suggestion if the counts are worth pinning now. Recommend this lands before #341.
| if not isinstance(qubit, IndexedIdentifier): | ||
| # physical qubit ("$n"): an absolute hardware index that | ||
| # belongs to no declared register, so nothing to consolidate | ||
| stmt_qubits.append(qubit) |
There was a problem hiding this comment.
[M1] Mixed declared/physical programs emit two address spaces — Maintenance · Medium
Follow-up issue, not a blocker on this diff — provenance note at the end.
Rationale: for a program that mixes declared registers with physical qubits, the emitted OpenQASM describes two address spaces but the qubit count collapses them into one, and nothing in the output says which is intended.
qubit[2] q; qubit[2] __PYQASM_QUBITS__;
h q[0]; ------> h __PYQASM_QUBITS__[0];
cx q[0], q[1]; cx __PYQASM_QUBITS__[0], __PYQASM_QUBITS__[1];
h $1; h $1;
module.num_qubits == 2. Internally $1 and __PYQASM_QUBITS__[1] are counted as the same qubit — _register_physical_qubit does num_qubits = max(num_qubits, phys_idx + 1) — but the emitted program never states that. A consumer sees a virtual register the hardware may map anywhere, plus an absolute reference to physical qubit 1. If they are the same qubit, the output should say so; if they are not, num_qubits under-counts.
A second shape makes it starker: qubit[2] q; h q[0]; h $5; emits qubit[6] __PYQASM_QUBITS__; with h __PYQASM_QUBITS__[0]; and h $5;, num_qubits == 6 — a six-slot register of which one slot is used, sized by a physical index the register does not contain.
Provenance: running both shapes on main through the measure/reset paths (qubit[2] q; h q[0]; c = measure $1; and qubit[2] q; h q[0]; reset $5;) gives identical output and identical counts. This is pre-existing #325 semantics, not something this diff creates.
Change requested: none here. Worth a separate issue to settle what consolidate_qubits=True should mean for mixed programs. Three options, author's call:
- Map physical operands into the consolidated register using the same offset scheme, so the whole program speaks one address space — arguably what "consolidate" implies;
- Raise a
ValidationErrorwhen a program mixes declared registers with physical qubits underconsolidate_qubits=True, rather than emitting something ambiguous; - Keep current behaviour, document the semantics explicitly, and fix
num_qubitsso it does not conflate the two address spaces.
There was a problem hiding this comment.
Follow-up filed as #353. No code change here, per your call.
Reproduced both shapes on this branch first:
qubit[2] q; h q[0]; cx q[0],q[1]; h $1; -> qubit[2] __PYQASM_QUBITS__; ... h $1; num_qubits=2
qubit[2] q; h q[0]; h $5; -> qubit[6] __PYQASM_QUBITS__; ... h $5; num_qubits=6
#353 records all three options verbatim, the _register_physical_qubit mechanism behind the collapsed count, and your provenance finding that the measure/reset paths on main give identical output and counts. It also folds in L1.
The description now has a "Scope: what this does not settle" section pointing at it, rather than leaving the question unmentioned.
| cz $2, $1; | ||
| """ | ||
| expected_qasm = """OPENQASM 3.0; | ||
| qubit[5] __PYQASM_QUBITS__; |
There was a problem hiding this comment.
[L1] Unreferenced consolidated declaration — Implementation · Low
Rationale: this expected output is the clearest instance of the case. For a program with no declared registers at all (h $1;, and likewise the box and crz shapes), the output still carries a qubit[N] __PYQASM_QUBITS__; declaration that no statement references. It parses fine, but a device backend may allocate against it.
Change requested: consider suppressing the consolidated declaration when nothing was consolidated into it — i.e. sum(global_qreg_size_map.values()) == 0 — in _add_pyqasm_qubit_register in visitor.py. Reasonable to fold into the M1 follow-up rather than doing it here. Left as prose rather than a suggestion because the edit lands in a file this PR does not touch.
There was a problem hiding this comment.
Folded into #353 rather than done here, as you suggested — the edit lands in visitor.py, which this PR does not touch.
Confirmed the case: h $1; alone emits qubit[2] __PYQASM_QUBITS__; that no statement references. #353 records the sum(global_qreg_size_map.values()) == 0 condition in _add_pyqasm_qubit_register and notes that suppressing it falls out of whichever address-space option gets chosen.
| """ | ||
| result = loads(qasm, device_qubits=5) | ||
| result.unroll(consolidate_qubits=True) | ||
| check_unrolled_qasm(dumps(result), expected_qasm) |
There was a problem hiding this comment.
[L2] Test pins the semantics without stating the count — Maintenance · Low
Rationale: test_physical_qubits_only asserts the unreferenced qubit[5] __PYQASM_QUBITS__; declaration as expected output, which pins the current semantics in place. That is fine as a regression guard for the crash, but if the address-space question in M1 is resolved in either direction, this test has to change with it — and right now the intended qubit count is implicit in a string.
Change requested: state the count explicitly, here and in test_physical_qubits_are_not_consolidated, so the eventual semantics decision becomes a visible, deliberate test edit.
| check_unrolled_qasm(dumps(result), expected_qasm) | |
| check_unrolled_qasm(dumps(result), expected_qasm) | |
| assert result.num_qubits == 5 |
There was a problem hiding this comment.
Resolved in ffd7a9a — counts asserted in both tests, and #353 referenced from each so the eventual semantics change is a deliberate edit.
One correction to the suggestion: the count is 3, not 5, in both tests. The 5 in qubit[5] __PYQASM_QUBITS__; comes from device_qubits=5, not from the qubit count — test_physical_qubits_only uses $1 and $2, so num_qubits == max(phys_idx + 1) == 3, and the consolidated case is 2 declared slots against $2, also 3.
That is a third number in play beyond the two your finding names, so I added it to #353 and left a comment on each assertion saying where each number comes from — otherwise assert result.num_qubits == 3 next to a qubit[5] declaration reads like a bug.
State num_qubits explicitly in test_physical_qubits_are_not_consolidated and test_physical_qubits_only so the mixed address-space semantics tracked in #353 become a deliberate test edit rather than an implicit string change. Both count 3, not the 5 in the declared register -- that comes from device_qubits (L2).
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Approved.
Re-verified at ffd7a9a: the h $1; repro and the mixed declared/physical shapes all succeed and round-trip, and declared-register consolidation is unregressed. Suite 638 passed, 4 skipped; CI 26/26.
Good catch on the count in L2 — num_qubits == 3, not 5; the 5 comes from device_qubits=5, not the qubit count. Confirmed on both tests. The suggestion as written would have failed, and the comment explaining where each number comes from is worth having.
M1 and L1 tracked in #353.
…cal-qubits # Conflicts: # CHANGELOG.md
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Re-approving after merging origin/main (which now carries #345) into the branch. The only conflict was CHANGELOG.md — both entries kept, no source change. Suite on the merged branch: 647 passed, 4 skipped.
3 participants
Summary of changes
Closes #343.
Problem
unroll(consolidate_qubits=True)crashed withAttributeError: 'str' object has no attribute 'name'for any gate applied to a physical qubit (h $1;).consolidate_qubit_registersassumed every gate operand is anIndexedIdentifierand readqubit.name.name; a physical qubit survives unrolling asIdentifier("$1"), whose.nameis a plain string.Fix
Gate operands that are not
IndexedIdentifierpass through unchanged. Physical qubits are absolute hardware indices that belong to no declared register, so there is nothing to consolidate — this matches whatmeasure,resetandbarrieralready do with them (#325).The consolidated
__PYQASM_QUBITS__declaration is still emitted for programs that mix declared registers with physical qubits; only the physical operands are left as written.Scope: what this does not settle
Fixing the crash exposes a design question it deliberately leaves alone. A mixed program emits two address spaces — a consolidated virtual register plus absolute physical references — while
num_qubitscollapses them into one, and a program with no declared registers still gets an unreferenced__PYQASM_QUBITS__declaration. Both are pre-existing #325 semantics, reproducible onmainthrough themeasure/resetpaths, which theAttributeErrorpreviously masked for gate operands. Tracked in #353 with three options for resolving it.The two consolidation tests now assert
num_qubitsexplicitly, so whichever way #353 goes, the change lands as a visible test edit rather than a quiet string diff.Tests
test_physical_qubits_are_not_consolidated— declared register plus$2across gate,measureandczoperands; consolidates the declared half, leaves$2as written,num_qubits == 3.test_physical_qubits_only— the issue repro shape, nothing to consolidate,num_qubits == 3.Worth noting for whoever reads those counts: neither is the
5in the emittedqubit[5] __PYQASM_QUBITS__;declaration — that number comes fromdevice_qubits, which is a third quantity again. That mismatch is part of what #353 is for.