Fix barrier unrolling to preserve multi-qubit barrier statements#295
Open
Fix barrier unrolling to preserve multi-qubit barrier statements#295
Conversation
Support barrier merge/split operations in transformer, fix visitor handling of physical qubits, and add corresponding tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughBarrier unrolling now preserves multi‑qubit barrier directives as single statements. The visitor expands barrier ranges into explicit qubit indices; the transformer maps each qubit to device indices and rewrites qubit references. Tests and CHANGELOG updated to expect consolidated, comma‑separated barrier statements. Changes
Sequence DiagramsequenceDiagram
participant Parser as Visitor / Parser
participant Barrier as Barrier Statement
participant Transformer as Transformer
participant Emitter as Emitter / Output
Parser->>Barrier: parse barrier (identifiers, ranges, indexed ids)
Barrier-->>Parser: expanded qubit list (ranges -> explicit indices)
Parser->>Transformer: pass single expanded_barrier
Transformer->>Transformer: iterate qubits -> map to device indices
Transformer->>Transformer: rewrite qubit refs to __PYQASM_QUBITS__[…]
Transformer-->>Emitter: emit single consolidated barrier statement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pyqasm/transformer.py`:
- Line 508: The long call that passes _qubit_str.name, ind.value,
qubit_register_offsets, global_qreg_size_map exceeds the line-length limit;
split the arguments across multiple lines (or assign a temporary
tuple/list/kwargs) so each line stays under the formatter max length, e.g.,
break the argument list into one-per-line inside the enclosing parentheses where
this call occurs (look for the call site involving _qubit_str.name and ind.value
in transformer.py) and ensure you keep proper commas and indentation to satisfy
the linter.
- Around line 495-499: The current code mutates Identifier.name to embed slice
syntax (the block setting _qubit_id.name to "__PYQASM_QUBITS__[...]" when
handling barrier ranges), which breaks downstream consumers; instead, replace
that mutation by constructing an IndexedIdentifier whose index is a
RangeDefinition built from IntegerLiteral nodes (use IndexedIdentifier,
RangeDefinition, IntegerLiteral) so the AST retains a proper range node; modify
the branch that handles consolidate_qubits=True and unroll_barriers=False (the
branches that set _qubit_id.name for the three cases: full slice, open-ended
slice, and start:end slice) to create and assign an IndexedIdentifier containing
a RangeDefinition with appropriate IntegerLiteral start/end (or omitted for
open-ended), rather than changing Identifier.name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e92d3f6-bf8b-4503-9dbc-a450dc116df4
📒 Files selected for processing (6)
CHANGELOG.mdsrc/pyqasm/transformer.pysrc/pyqasm/visitor.pytests/qasm2/test_operations.pytests/qasm3/test_barrier.pytests/qasm3/test_device_qubits.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Member
Author
|
Building circuit composer on account. This update is needed from pyqasm to support the backend. So linking that issue here. This PR will close the issue because the bulk of the issue is being actively worked on elsewhere, and this is the only pyqasm related change needed (as far as I can tell) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pyqasm/transformer.py`:
- Around line 503-511: The loop that remaps indices (iterating _qubit_ind and
then ind) assumes each ind is an IntegerLiteral and accesses ind.value, which
fails for RangeDefinition nodes (e.g., q[0:2]); update the logic in
consolidate_qubit_registers (used when consolidate_qubits=True and
unroll_barriers=False) to guard against non-literal nodes: before accessing
ind.value, check the node type (e.g., isinstance(ind, IntegerLiteral)); if it's
an IntegerLiteral, call _get_pyqasm_device_qubit_index(_qubit_str.name,
ind.value, qubit_register_offsets, global_qreg_size_map) and assign back to
ind.value; if it's a RangeDefinition (or other range node), explicitly handle it
by remapping its bounds/contained indices to device indices (iterate its
start..end or child indices and replace those elements with remapped
IntegerLiteral nodes or update the range representation accordingly) instead of
directly accessing .value; apply the same guarded handling to the equivalent
loops used for measurement and reset remapping that call
_get_pyqasm_device_qubit_index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa6f2d82-5d5f-4134-b1f0-6c1bedea8b5d
📒 Files selected for processing (2)
src/pyqasm/transformer.pysrc/pyqasm/visitor.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pyqasm/visitor.py`:
- Around line 782-799: The consolidation logic incorrectly expands barrier
operands by comparing register names in barrier_qubits (using eq.name.name and
_global_qreg_size_map) which collapses aliases and over-consumes adjacent
operands; instead, for each original operand (op_qubit) call the expansion
routine (_get_op_bits or equivalent used upstream) and append that operand's
concrete list of qubit bits to consolidated_qubits, advancing expanded_idx by
the number of bits produced for that operand rather than scanning by name;
update the loop around expanded_idx/op_qubit to use the per-operand expanded
list and remove name-based comparisons (eq.name.name and qubit_name) so
constructs like "let a = q; barrier a;" and mixed slices "barrier q[0:2], q[3]"
expand correctly.
In `@tests/visualization/test_mpl_draw.py`:
- Around line 132-134: The test uses _check_fig(circ, fig) which only inspects
ax.texts and therefore misses barrier rendering (barriers are drawn as
collections/patches in src/pyqasm/printer.py); update the test to assert
directly on the Matplotlib artists created by mpl_draw by checking
fig.axes[0].collections and fig.axes[0].patches for the expected barrier artists
(or modify _check_fig to include those checks), or alternatively convert this
test into an mpl_image_compare style test to verify the rendered barrier
visually.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 904f7298-2e15-472c-9773-6ebdab088724
📒 Files selected for processing (3)
src/pyqasm/visitor.pytests/qasm3/test_device_qubits.pytests/visualization/test_mpl_draw.py
Replace name-based comparisons in _expand_barrier_ranges with per-operand expansion counting via _get_op_bits, fixing incorrect behavior for aliases and mixed same-register slices. Add barrier-specific artist assertions to test_draw_barriers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
barrier q[0], q[1], q[2]) instead of splitting into individual per-qubit barriersSummary by CodeRabbit