fix: record an external gate's own depth, not its skipped decomposition's - #359
fix: record an external gate's own depth, not its skipped decomposition's#359ryanhill1 wants to merge 2 commits into
Conversation
…on's _visit_external_gate_operation ran _visit_basic_gate_operation for validation only, but that call still updated qubit depths over the decomposition that was never emitted. Depth recording is now suppressed around the validation call and the external gate records its own depth, mirroring the existing custom-gate path. Affects both explicit external_gates and braket verbatim boxes. Fixes #352
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
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❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@Argus-Eye review |
This comment has been minimized.
This comment has been minimized.
![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 4 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: Ensure external gates record their own depth instead of the depth of a decomposition skipped during validation.
Stated acceptance criteria (from PR/issue — not independently verified):
- Suppress depth recording around the validation-only _visit_basic_gate_operation call.
- Record the external gate's own depth for explicit external_gates and Braket verbatim boxes.
- Record the external gate's own depth in the branch-qubit marking variant inside conditionals.
✅ Intent delivered
Argus reviewed 4 files and found no issues. Code looks good.
Simulated 2 scenarios — all pass.
🔢 124.9k tokens · $0.3314 total
| Stage | Tokens | Cost |
|---|---|---|
| Intent | 2.5k | $0.0000 |
| Triage | 2.7k | $0.0000 |
| Lead agent | 1.6k | $0.0000 |
| Review · bug_hunter | 20.7k | $0.0857 |
| Review · security | 20.4k | $0.0746 |
| Review · architecture | 20.5k | $0.0845 |
| Review · regression | 20.8k | $0.0866 |
| Review | 24.2k | $0.0000 |
| Acceptance | 1.2k | $0.0000 |
| Simulation | 10.4k | $0.0000 |
Contract: production/full · checked: bug_hunter, security, architecture, regression · review took 1m30s
Dashboard → · React 👎 to dismiss · Reply to any inline comment or use @argus-eye help to chat
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Verdict
Approve. The fix is correct, minimal, and well targeted. Suppressing depth recording around the validation-only _visit_basic_gate_operation call is the right mechanism, and the try/finally save-restore is more robust than the unconditional assignment the custom-gate path uses. Two Low-severity suggestions below; neither blocks merge.
The fix reaches further than the issue title suggests. Verified corrections beyond the single-crz repro:
Program (crz external / verbatim) |
base | this PR | statements emitted |
|---|---|---|---|
single crz |
12 | 1 | 1 |
two sequential crz, same qubits |
24 | 2 | 2 |
two crz, disjoint qubits |
12 | 1 | 2 |
crz in a for loop, 3 iterations |
36 | 3 | 3 |
crz in a plain (non-verbatim) box |
12 | 1 | 1 |
crz in a #pragma braket verbatim box |
12 | 1 | 1 |
crz inside a plain custom gate body |
12 | 1 | 1 |
Findings
- Low / Implementation — the new branching arm is untested and currently has no observable effect (inline on
src/pyqasm/visitor.py:1413) - Low / Maintenance — CHANGELOG wording overstates the custom-gate path's correctness (inline on
CHANGELOG.md:29)
No Critical, High, or Medium findings. No security surface: the change touches only an internal depth metric, adds no I/O, and introduces no new parsing or user-controlled path.
How this was tested
A detached worktree at 89453bcc was compared against origin/main (7c31308) by running both trees through the same battery.
- Issue #352 repro — confirmed base reports
depth() == 12for one emitted statement; the PR reports1. The value1was derived independently from the emitted program, not read off the PR. - Double-counting —
_broadcast_gate_operationrecords no depth, and no code after the new block does either. Two sequential external gates on the same qubits give2; on disjoint qubits,1; sharing one qubit,2. Counted exactly once. - Broadcast form —
crz(0.5) c, t;over registers passes a multi-subsetall_targets, a shape the custom-gate path never exercises. Depth is correct for each emitted chunk. - Flag safety — instrumented
_visit_external_gate_operationto record_recording_ext_gate_depthon entry and exit across nested external gates, external gates inside plain and external custom gate bodies, three invalid external gate calls that raiseValidationError, and an injectedRuntimeErrorinside the validation call. Zero leaks in every case; thefinallyholds. - Regression sweep — a 10-program corpus with no external gates (Bell, GHZ, decomposable gates, custom gates, measurement, physical qubits, barrier, reset, broadcast,
pow @) reports identical depth on base and PR, for bothdecompose_native_gates=TrueandFalse.depth(decompose_native_gates=False)is unchanged across every case in the battery. - Test suite — 718 passed on the PR versus 714 on base (the 4 new tests), same 4 skips. Two
tests/cli/test_cli_commands.pyfailures reproduce identically on base and are unrelated to this change. - Lint and CI —
black --checkclean on all three changed files; all 26 GitHub checks pass.
Next steps
- Add a conditional-path test, as described inline on
visitor.py:1413. - Apply the one-line CHANGELOG suggestion.
- Optionally open a follow-up issue for the nested external custom gate gap described inline on
CHANGELOG.md:29— pre-existing, out of scope here.
| if gate_name not in self._custom_gates: | ||
| if not self._in_branching_statement: | ||
| self._update_qubit_depth_for_gate(all_targets, ctrls) | ||
| else: |
There was a problem hiding this comment.
Type: Implementation
Severity: Low
Rationale: This branching arm is reachable but has no observable effect today, and no test covers it.
Evidence: replacing the whole else body with pass leaves the full PR suite green — 718 passed, 4 skipped, and only the two pre-existing test_cli_commands.py failures that also fail on base. Depth was also unchanged across six conditional programs (external gate in if, ctrl @ external in if, external plus a second gate in if, external on qubits idle outside the branch, external gate inside an external custom gate in if, and a nested if) — every one reports 2 on base, on this PR, and with the arm deleted.
The reason is that the arm duplicates marking the validation-only call already performs. _recording_ext_gate_depth guards only _update_qubit_depth_for_gate (visitor.py:1098); it does not guard _mark_branch_qubit. So the suppressed _visit_basic_gate_operation call still reaches the branch-marking block at visitor.py:1201-1208 and marks the same qubits.
This is worth keeping rather than deleting. The arm becomes load-bearing the moment the suppression is extended to cover branch marking too — which is the natural reading of "suppress depth recording" and a plausible future refactor. Right now nothing pins that behaviour, so a later change could silently regress conditional depth with no test failing.
Change Requested: Add a test that fixes the conditional contract, so the arm is covered and a future change to the guard cannot regress it silently. For example, in tests/qasm3/test_depth.py, assert that a crz inside an if block reports the same depth whether or not crz is external (both are 2 today), and ideally one case where the external gate acts on qubits untouched outside the branch.
| ### Removed | ||
|
|
||
| ### Fixed | ||
| - Fixed external and verbatim-box gates counting the depth of the decomposition they skipped: `unroll(external_gates=["crz"])` on a single `crz` reported `depth() == 12` while emitting one statement. An external gate now records its own depth, like the custom-gate path already did. ([#352](https://github.com/qBraid/pyqasm/issues/352)) |
There was a problem hiding this comment.
Type: Maintenance
Severity: Low
Rationale: "like the custom-gate path already did" reads as though the custom-gate path is correct in general. It is not, for nested external custom gates — the same user-visible symptom this entry describes still occurs there.
Evidence, identical on base and on this PR (so pre-existing and not a regression from this change):
gate inner a, b { crz(0.5) a, b; }
gate outer a, b { inner a, b; h a; }
qubit[2] q;
outer q[0], q[1];With external_gates=["outer"] this emits exactly one statement, outer q[0], q[1];, but reports depth() == 13. With external_gates=["inner", "outer"] it reports 2 for the same single statement. A single-level external custom gate correctly reports 1, which is why the existing tests pass.
The cause is precisely the pattern this PR improved on. _visit_custom_gate_operation assigns the flag unconditionally at visitor.py:1285 and clears it unconditionally at visitor.py:1317-1318, with no save-restore. The inner custom gate therefore clobbers the outer gate's flag, and the outer gate never records its own depth. The new code here avoids exactly that by saving prev_recording and restoring it in a finally — applying the same pattern at :1285 would close the remaining gap.
Change Requested: Soften the wording so the entry does not claim the custom-gate path is already fully correct. Consider a follow-up issue for the nested case; fixing it here would widen the PR beyond #352.
| - Fixed external and verbatim-box gates counting the depth of the decomposition they skipped: `unroll(external_gates=["crz"])` on a single `crz` reported `depth() == 12` while emitting one statement. An external gate now records its own depth, like the custom-gate path already did. ([#352](https://github.com/qBraid/pyqasm/issues/352)) | |
| - Fixed external and verbatim-box gates counting the depth of the decomposition they skipped: `unroll(external_gates=["crz"])` on a single `crz` reported `depth() == 12` while emitting one statement. An external gate now records its own depth, matching how a single-level external custom gate is already handled. ([#352](https://github.com/qBraid/pyqasm/issues/352)) |
3 participants
Fixes #352
_visit_external_gate_operationcalls_visit_basic_gate_operationfor validation only, but that call still ran_update_qubit_depth_for_gateover the decomposition that was never emitted — sounroll(external_gates=["crz"])on a singlecrzreporteddepth() == 12while emitting one statement. Since #341 the same path handles every decomposable gate inside a#pragma braket verbatimbox.Depth recording is now suppressed around the validation-only call (the same
_recording_ext_gate_depthmechanism the custom-gate path uses), and the external gate records its own depth — including the branch-qubit marking variant inside conditionals.