Fix box duration validation to track delays per qubit timeline#330
Fix box duration validation to track delays per qubit timeline#330ryanhill1 wants to merge 4 commits into
Conversation
Box duration validation summed all delay durations inside a box into a single accumulator regardless of target qubit, rejecting valid programs whose parallel per-qubit timelines fit the declared duration. The same schedule written as one broadcast delay was accepted, so semantically identical programs validated differently. Delays are now accumulated per qubit key on a stack of per-box frames: the box only needs to fit the busiest single qubit timeline, sequential delays on one qubit still accumulate and reject correctly, and a nested box contributes its declared duration to the enclosing box's timelines (previously the accumulator was reset when an inner box closed, losing all inner delay accounting). The validation error now names the offending qubit. Fixes #329
|
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:
📝 WalkthroughWalkthroughBox duration validation now accumulates delays per qubit, accounts for nested boxes, reports offending qubits, and adds regression coverage. Duplicate-qubit detection uses a shared identity-key helper for physical and indexed qubits. ChangesBox timing validation
Qubit identity extraction
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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✅ All modified and coverable lines are covered by tests. 📢 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: 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 `@src/pyqasm/analyzer.py`:
- Around line 266-270: Update extract_duplicate_qubit with the return annotation
tuple[str, int] | None, and annotate its qubit_set local variable as
set[tuple[str, int]]. Keep the existing duplicate-detection behavior 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 821d1321-ef4f-4f62-b42c-b9a0e5905d88
📒 Files selected for processing (4)
CHANGELOG.mdsrc/pyqasm/analyzer.pysrc/pyqasm/visitor.pytests/qasm3/test_box.py
| qubit_key = Qasm3Analyzer.extract_qubit_key(qubit) | ||
| if qubit_key in qubit_set: | ||
| return qubit_key | ||
| qubit_set.add(qubit_key) | ||
| return None |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add complete type annotations to duplicate detection.
The changed method now depends on a typed (str, int) key, but extract_duplicate_qubit still has no return annotation and qubit_set is untyped. Add -> tuple[str, int] | None and set[tuple[str, int]] so mypy can validate this refactored path.
🤖 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/analyzer.py` around lines 266 - 270, Update
extract_duplicate_qubit with the return annotation tuple[str, int] | None, and
annotate its qubit_set local variable as set[tuple[str, int]]. Keep the existing
duplicate-detection behavior unchanged.
Source: Coding guidelines
| if _box_time_var and box_duration_val and delay_frame: | ||
| # Delays on different qubits run in parallel, so the box only | ||
| # needs to fit the busiest single qubit timeline. | ||
| max_qubit_key = max(delay_frame, key=lambda k: delay_frame[k]) |
There was a problem hiding this comment.
Why not just keep the (name) of the register with the largest duration in the box as the key rather than the (name,index) pair? Eg. -
box[350ns] { delay[200ns] q[0]; delay[400ns] q[1]; delay[300] q[0]; delay[600] q_new[0]; }
would only require { 'q' : 500, 'q_new' : 600 } to determine whether the operations fit in the box duration. We can probably just make a simple dict rather than list of dict.
Do you see a use case for the per-qubit durations in any later processing stages?
There was a problem hiding this comment.
The per-qubit keys are load-bearing — the 500 in your example is q[0]'s sequential total (200 + 300), i.e. the max over per-qubit sums. A register-keyed dict has nowhere to hold q[0] = 200 while q[1] = 400 arrives, so at the third delay there's no way to know which running total the 300 belongs to. Both collapse strategies lose it:
- sum into
'q'→ 200+400+300 = 900 (that's the bug this PR fixes) - max into
'q'→ max(200, 400, 300) = 400, never 500
The max variant also regresses a test in this PR:
box[300ns] { delay[200ns] q[0]; delay[200ns] q[1]; delay[150ns] q[0]; }q[0]'s timeline is 350ns and must be rejected; register-max gives 200ns and silently accepts it — a false accept, worse than the false reject we started with.
The register-level max is fine as an output, just not as an accumulator — and that's what the code already does: it accumulates per qubit, then takes the max at box close for the comparison and the error message.
On list[dict]: that's a stack, not parallel data — one frame per open box, so a nested box doesn't clobber its parent's timelines. Depth is bounded by box nesting. Folding depth into a single dict's key would be strictly worse.
On later stages: I'd argue it's required for correctness here regardless of future use. That said, per-qubit timelines are the natural input to anything scheduling-related later (stretch resolution, inferring an undeclared box duration), so I don't expect this to be throwaway state.
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Thanks for fixing this @ryanhill1 ! Gave one comment regarding the data structure being used, but the idea is correct
3 participants
Summary
Fixes #329.
Box duration validation summed the durations of all
delaystatements inside aboxinto a single accumulator, regardless of which qubits they target. Delays on disjoint qubits execute in parallel, so this rejected valid programs whose per-qubit timelines fit comfortably within the declared duration — while accepting the same physical schedule written as a single broadcast delay:box[300ns] { delay[200ns] q[0]; delay[200ns] q[1]; } // rejected: "400.0ns > 300.0ns" box[300ns] { delay[200ns] q; } // accepted (identical schedule)Changes
visitor.py): the flat_total_delay_duration_in_boxcounter is replaced with a stack of per-box frames mapping(register, index)→ accumulated delay on that qubit's timeline. A broadcast delay (no qubit args) now contributes to every qubit in scope, fixing the broadcast/per-qubit inconsistency. Validation compares the busiest single qubit timeline against the box duration.Total delay duration value '350.0ns' on qubit 'q[0]' should be less than 'box[300.0ns]' duration.analyzer.py): extractedQasm3Analyzer.extract_qubit_key()fromextract_duplicate_qubit()so both share the qubit-identity logic (handles physical$nqubits and indexed identifiers).Semantics
box[300ns] { delay[200ns] q[0]; delay[200ns] q[1]; }→ valid (each timeline is 200ns)box[300ns] { delay[200ns] q[0]; delay[200ns] q[0]; }→ still invalid (q[0]'s timeline is 400ns)box[300ns] { delay[200ns] q; }→ still valid (each qubit's timeline is 200ns)box[500ns] { box[400ns] { delay[100ns] q[0]; } delay[150ns] q[0]; }→ now invalid (400 + 150 = 550ns on q[0]; previously the inner box's time was dropped entirely)Testing
Test-driven: the four new/updated cases were written first and fail against the previous implementation (
5 failed, 15 passedontests/qasm3/test_box.pybefore the fix). With the fix:tests/qasm3/test_box.py: 21 passed (new: parallel-disjoint-delays regression, broadcast/per-qubit consistency, same-qubit sequential overflow, nested-box propagation; updated: the two error-message expectations now include the qubit)black,isort, andpylintclean on changed filesSummary by CodeRabbit
boxduration validation to track delay time independently for each qubit.