IBM QAOA: dead-code cleanup, full-recursion Nautilus campaigns

[anurag-r20]

Summary

Based on main (18 files, +8824/-4922).

• Removes 34 dead top-level names (29 from src/simulation_validation.py,
  5 from src/utils.py) with zero remaining callers anywhere in the tree —
  backend-choice suite, hardware-alignment stats, WS campaign builders,
  unused plotting helpers. Verified this doesn't affect reproducing
  Analysis.ipynb or Simulation_Method_Validation_and_WS.ipynb.
• Fixes a real misalignment bug in prepare_training_bricks_data: a
  positional .sem().values assignment is replaced with an explicit merge
  on ["job_p", "method_base"].
• Fixes a double-counting bug in run_fa_pss_exact_points: after a
  cached/resumed grid point is skipped, the next actually-computed point
  cold-starts (trains its full budget from scratch, since warm-start angles
  aren't cached) but its own full cost was being stacked on top of the
  cumulative counters carried over from the cached row, inflating
  total_training_shots for every point downstream of a resume. Fixed via
  _reset_cumulative_cost_on_cold_start, guarded by a scenario test
  reproducing the exact repro from review (n_values=[10,20,30], resumed
  after a cached N=20 gives the same cumulative total as an uninterrupted
  run, not the inflated 5000 vs. the correct 3000). The one committed
  campaign this affected (heavy_hex_144_I_full_p7_expanded, Interp/
  I_MPSAer) was confirmed to have 10/210 resumed grid columns carrying the
  inflated prefix and was regenerated end-to-end with the fix; the
  regenerated data was verified clean (zero inflated-offset rows across all
  30 instances × 343 grid columns).
• snap_actionable_fit_to_feasible_grid's fit for per-(N,M,Q) actionable
  curves is now a monotonic (non-decreasing) log-log power law instead of
  isotonic regression (PAVA): PAVA pools locally-nonmonotone points into
  flat plateaus, which collapses parameters like M(T) — where many (N,M)
  splits trade off at similar budgets — into a plateau that doesn't track
  the real scaling trend. The power-law fit is per-(N,M,Q) column (not
  pooled across columns) and clips its fitted slope to ≥0.
• build_bound_circuit_simulator: builds one AerSimulator for reuse
  across an (N, M) grid sweep instead of rebuilding it per iteration.
• Exact-point checkpointing in generate_pss_exact_points is now batched
  (_CHECKPOINT_BATCH_SIZE = 10) instead of writing on every new row.
• line_to_full instance generation now symmetrizes its random sign matrix
  (was silently asymmetric).
• run_stochastic_benchmark_pss gains resource_match_bins (default 150),
  which snaps sb.interp_results onto a coarser shared grid before
  recipe/evaluate matching. This changes campaign results by default: it
  works around training.evaluate_single matching resource values by exact
  float equality (train/test instances land on independent resource grids,
  so without snapping the overlap is small and arbitrary from run to run).
  Tracked as a follow-up against the core package in training.evaluate_single matches resource values by exact float equality #85; the unseeded
  bootstrap resampling this interacts with is tracked in Bootstrap resampling is unseeded, making campaign re-runs non-reproducible #86.
• run_job.sh's zero-training method routing now reads
  ZERO_TRAINING_METHODS out of src.simulation_validation at run time
  instead of maintaining a second copy of the list in shell.
• Adds Nautilus jobs/scripts for full-recursion Interp*/I_MPSAer campaigns
  at p=7 and p=9 (sharded runs, cleanup/finalize/setup jobs), an LR
  angle-optimization tier at p=5/p=9, a linear_ramp_no_opt (no
  re-optimization) tier wired through ZERO_TRAINING_METHODS, and
  extended-Q-grid FA_no_opt/PT re-run scripts.
• Adds hardware-calibrated multi-strategy window sticker and Pareto frontier
  overlay cells to Simulation_Method_Validation_and_WS.ipynb. Fixed-Angles*
  and Fixed-Angles† both stay on plt.cm.Blues with disjoint bands (0.55–
  0.92 vs. 0.15–0.42) rather than colliding on overlapping stretches of the
  same colormap; Param. Transfer uses Greys, the Linear Ramp tiers use
  YlOrBr, Interp uses Greens — one legend handle per method family
  alongside the per-family colorbars. The extend_curves_to_xlim flat-hold
  segment is drawn dashed at half alpha so it isn't mistaken for measured
  data.
• Adds a best-bitstring real-hardware Pareto overlay validating the
  simulated actionable-prescription frontier against real ibm_boston
  execution on the same ten test instances: the hardware curve's QPU
  execution time is recalibrated onto the identical paper-measured
  shot-rate basis (arXiv:2606.05311v1) the simulated Noiseless/
  Noise-Corrected curves already use, rather than the raw, even-split-
  per-job JSON-derived QPU time — only QPU time is rescaled, since
  classical training cost isn't shot-rate dependent. (QPU time turns out to
  be under 1% of total real-hardware resource cost for a representative
  point, so the recalibration is correct but doesn't move the curve much;
  classical training dominates the real-hardware budget.)
• warnings.warn's default formatter prints the absolute source-file path
  of the call site, bypassing the existing dataframe-path scrubbing
  entirely (e.g. src/interpolate.py's duplicate-resource warning leaked a
  contributor's local path into committed cell output). Added a
  warnings.formatwarning override in the notebook's config cell that
  relativizes the filename the same way the dataframe scrubbing does.

Does not include AUDIT_REPORT.md or the paper draft — dropped per
review feedback (the audit's findings are already reflected in the diff
itself, and a point-in-time audit shipped alongside its own fixes goes
stale on arrival; the paper draft was withdrawn separately).

Deferred

• RecursionTrainer's nested evaluator_init overwrite vs. .setdefault()
  question, deferred 2026-08-15 — tracked in RecursionTrainer nested evaluator_init: resolve overwrite vs .setdefault() semantics #87.

Test plan

• pytest tests/ -q — 498 collected (494 pass + 4 conditionally skip in
  a qiskit-aer-less environment; all 498 pass locally with
  qiskit-aer installed)
• flake8 --select=F401,F841,E9,F63,F7,F82 on touched Python files — clean
• Mutation-tested the resume-fix regression test: fails correctly
  (5000 != 3000) with the production reset call deleted
• Regenerated heavy_hex_144_I_full_p7_expanded end-to-end with the
  resume-double-counting fix; verified clean across all 30 instances
• Simulation_Method_Validation_and_WS.ipynb re-executed end-to-end
  with all plotting/layout changes: 14/14 code cells run, zero error
  outputs, monotonically increasing execution counts (1-14)

[anurag-r20]

@bernalde

[bernalde]

Thanks for pulling the audit fixes out into their own PR — the dead-code sweep is well evidenced, and the prepare_training_bricks_data change (positional .sem().values → merge on ["job_p", "method_base"]) is a real misalignment bug worth catching on its own.

Verified locally at 9c76ae7:

• pytest tests/ -q → 333 passed (matches your test plan)
• flake8 --select=F401,F841,E9,F63,F7,F82 on the touched Python files → clean
• Both notebooks: 13 and 23 code cells, monotonic execution counts, zero error outputs
• Every top-level name removed from src/simulation_validation.py / src/utils.py (34 by my count) has zero remaining references anywhere in the tree — the "no callers" claim holds
• GitHub checks on this head: 6/6 green (test 3.10/3.11/3.12, tutorial-smoke, integration-tests, coverage-summary)

Four things block merge.

Blocking

1. This PR targets main but is stacked on the still-open #50

git merge-base main 9c76ae7 is 2680ac7 — the same merge base as #50 — and #50's head is an ancestor of this head. The split:

diff	files	insertions
main..#50	82	45,683
main..#84 (what GitHub shows here)	90	46,715
#50..#84 (what this PR actually adds)	15	2,394 (−1,362)

80 of the 82 commits and ~97.8% of the added lines belong to #50. Merging this as-is lands all of #50 into main before #50's own review concludes.

Please retarget the base to QAOA_Parameter_Setting_IBM, or hold this until #50 merges and then rebase. Either way the review surface becomes the 15 files you actually changed.

This is not cosmetic. Because the pull-files endpoint truncates positionally on a diff this size, src/simulation_validation.py, src/utils.py, run_job.sh and run_prepare_pss_campaign.py all return patch: null with +0/-0 — GitHub renders no hunks for them, so none of your actual code changes can receive an inline comment. That is why the findings below are in the review body rather than anchored on the lines they describe. Retargeting fixes that too.

2. Fixed Angles* and Fixed Angles† render as near-identical colours, and the legend that used to separate them was removed

_FAMILY_CMAP_SPEC assigns FA_star → (Blues, 0.38, 0.92) and FA_dagger → (Blues, 0.32, 0.58). Those ranges overlap on 0.38–0.58, so the two families draw from the same stretch of the same colormap. Running _build_family_color_map over the 14 method labels actually present in Simulation_Method_Validation_and_WS.ipynb:

d=0.051  'Fixed Angles$^\star$ (p=5)'  vs  'Fixed Angles† (p=3)'
d=0.092  'Fixed Angles$^\star$ (p=5)'  vs  'Fixed Angles† (p=2)'
d=0.113  'Fixed Angles$^\star$ (p=6)'  vs  'Fixed Angles† (p=6)'

(RGB Euclidean distance; for reference the smallest pre-existing cross-family gap is 0.194.) The last row is the damaging one: at the same depth p=6, full COBYLA optimisation and no optimisation are drawn in effectively the same blue — and that contrast is the point the figure exists to make.

Previously each method base got its own legend handle, so colour similarity was recoverable. This PR drops method_handles in favour of per-family colorbars, which leaves colour as the only discriminator, and the two Blues colorbars sit side by side.

Concrete fix — I swept the candidates against those same 14 labels:

FA_dagger colormap	worst cross-family distance	colliding pair
Blues (0.32, 0.58) — as merged	0.051	FA* vs FA†
BuPu	0.050	FA* vs FA†
Oranges	0.082	FA† vs Linear Ramp
Purples	0.111	FA† vs Param. Transfer
PuRd	0.160	FA† vs Param. Transfer
Reds / RdPu / copper	0.194	FA* vs Param. Transfer

plt.cm.Reds (or RdPu/copper) at (0.35, 0.85) raises the worst separation to 0.194, which is the ceiling already imposed by the pre-existing FA*/Param.-Transfer pair — roughly 4× the current worst case. Note Purples alone does not fix it; it just moves the collision to FA† vs Param. Transfer.

I would also keep one legend handle per method family alongside the colorbars, so the figure does not depend on colour discrimination alone.

3. resource_match_bins=150 changes campaign results by default, and works around a defect in the core package

run_stochastic_benchmark_pss gains resource_match_bins: int | None = 150, which snaps sb.interp_results onto a 150-point grid before the recipe/evaluate matching. The premise checks out — src/training.py:199 evaluate_single does match on exact float equality:

df_eval[df_eval[resource_col] == recipe[resource_col]]

Three problems with fixing it here:

1. It is on by default, so it silently coarsens the ~1000-point shared grid to 150 for every caller. Anyone rerunning a campaign gets different numbers than the committed notebook outputs, with nothing in the PR description flagging that.
2. The defect is in this repository's own core package, not a third-party dependency. Papering over it in one example leaves evaluate_single broken for every other consumer. Please open an issue against training.evaluate_single for tolerance-based (or binned) resource matching, and link it from the comment here.
3. The comment states that "bootstrap resampling below isn't seeded", so "the same campaign can yield wildly different numbers of surviving actionable-fit rows on separate runs." That is a reproducibility problem in its own right and deserves its own issue rather than only a mitigating workaround.

At minimum: document the parameter in the docstring, state the default in the PR description, and link the two follow-up issues.

Nonblocking

4. Title and description no longer match the head

The head commit 9c76ae7 ("Remove paper draft files from this PR") deletes resource_cost_extension.tex, resource_cost_extension_preview.pdf and v1.tex (−2,014 lines), but the title still ends "and paper draft" and the fifth summary bullet still advertises them. Also, the first bullet says 21 removed functions/constants; comparing top-level names between #50 and this head gives 29 from simulation_validation.py plus 5 from utils.py = 34.

Separately, several substantive changes are not described at all: build_bound_circuit_simulator (reuses one Aer backend across a grid sweep), exact-checkpoint batching (_CHECKPOINT_BATCH_SIZE = 10), the symmetrised sign matrix in line_to_full instance generation, the resource_match_bins change above, and the run_job.sh zero-training routing. Those are the changes a reviewer most needs pointed out.

5. No test covers any of the new logic

_pareto_envelope_and_owner, _build_family_color_map, _detect_method_family, _label_depth, build_bound_circuit_simulator and the resource_match_bins snapping have zero references under tests/. _pareto_envelope_and_owner in particular is pure, deterministic, dependency-free array logic — a handful of cases (all-NaN column, single-point entry, running-max persistence after a method's data ends) would pin the behaviour cheaply. _detect_method_family is worth pinning too, since a label that matches none of its branches falls through to s[:20], which is not in _FAMILY_ORDER and therefore gets no colorbar.

6. ZERO_TRAINING_METHODS is duplicated across Python and shell

simulation_validation.py:56 and run_job.sh:71 now hold the same three-element list in two languages. They agree today, but Bug 2 in your own audit report is precisely this pair drifting apart. Worth having run_job.sh read the list out of Python (e.g. python -c 'import ...; print(" ".join(sorted(ZERO_TRAINING_METHODS)))') so it cannot drift again.

7. extend_curves_to_xlim=True draws a flat hold with no visual cue

Both notebook call sites pass extend_curves_to_xlim=True, which repeats each curve's last value out to the right edge. The Pareto/background computation correctly uses the natural pre-extension series — that part is handled well and the comment explaining it is good. But the drawn line is indistinguishable from measured data, so a reader sees a plateau where there are simply no measurements. Consider drawing the extended segment dashed or at lower alpha.

Question

8. Overwriting the nested evaluator_init in RecursionTrainer

The new block replaces the previous chi-only propagation:

nested_init = trainer_init.setdefault("trainer_init", {})
if "evaluator" in nested_init:
    nested_init["evaluator"] = "MPSAerSampleEvaluator"
    nested_init["evaluator_init"] = dict(evaluator_init)

Two behaviour changes I want to confirm are intended, since the method configs live in the sibling QAOA-Parameter-Setting checkout and I can't check them from here:

• The old code used mps_args.setdefault(...), preserving anything a config had set for the nested evaluator. This assigns over the whole evaluator_init, so a config that deliberately set a different chi (or other MPS args) for the recursion evaluator now has it discarded. Deliberate?
• .get(...) became .setdefault(...), which fixes a real latent bug (the old call mutated a throwaway dict), but it also means a RecursionTrainer config with no nested trainer_init now gets one created carrying minimize_args/COBYLA. The outer block guards on trainer_name in {"ScipyTrainer", "TQATrainer"}; the nested one applies cobyla_maxiter unconditionally. Is the nested trainer always a ScipyTrainer in practice?

---

Blocking: 4 (1 inline, 3 in this body) · Nonblocking: 4 · Questions: 1 · Total: 9

Tests run at this head: pytest tests/ -q → 333 passed; flake8 --select=F401,F841,E9,F63,F7,F82 on touched files → clean; flake8 src --select=E9,F63,F7,F82 → 0; notebook execution-state audit → 0 errors, 0 unexecuted cells, monotonic counts. GitHub checks on 9c76ae7: 6/6 SUCCESS, none skipped. These are PR-head results; I did not evaluate the merge result, since finding 1 means the merge result would also carry all of #50.

Merge readiness: not ready. I would not merge this until the blocking issues above are addressed. The base retarget (finding 1) is the one to do first — it changes what the rest of this review is even looking at, and it restores inline commenting on your code.

[anurag-r20]

Re: the RecursionTrainer nested evaluator_init question (gh-review-pr:finding=nested-recursion-evaluator-init).

Leaving this code as-is rather than changing the overwrite/.setdefault() behavior. This is the exact code that generated the Interp/I_MPSAer results in this PR and the associated Nautilus campaign — changing it now would make those committed results not reproducible from the current code. Both behavior questions you raised are real and worth resolving, but that'll happen in a follow-up rather than in this PR, once we can check the actual method configs in the sibling QAOA-Parameter-Setting checkout rather than guessing from this repo alone.

[anurag-r20]

@bernalde

[bernalde]

Targeted re-review at f3c36db, superseding my review at 9c76ae7. The base merge and the color work landed well; CI is red on one item that has now been open across four rounds, and two commits brought in a methodology change I have not reviewed before.

@anurag-r20 — the concrete asks are the four numbered items below. The first is the only one blocking merge.

Resolved since 9c76ae7

• Base merge — c1e906ba merged main (with #50) into the branch. git merge-tree against main now exits 0 with no conflicts, and mergeable: true.
• Colour collision — verified after the merge rather than assumed, since a base merge can move a fix's dependencies. FA_star (0.55, 0.92) / FA_dagger (0.15, 0.42) on Blues, non-overlapping: worst cross-family separation is 0.172, up from the 0.051 I flagged. Splitting the ranges keeps the published QPS_METHOD_COLORS convention and fixes the collision, which is better than either the Reds swap I originally prescribed or the flat revert. I also re-ran the mutation at the merged head — restoring the overlapping ranges reddens test_fa_star_and_fa_dagger_do_not_collide and leaves its three siblings green, so the guard still guards.
• AUDIT_REPORT.md, resource-match-bins-default (issues #85/#86), zero-training-methods-duplication, extend-curves-flat-hold, and the title/body rewrite all remain addressed.

1. Blocking — the qiskit guard (see the inline comment)

Fourth round for this one, and CI can finally see it: 4 failed / 471 passed on f3c36db, and the same four failures on c1e906ba and a609fac. Details and a verified fix are in the inline comment on tests/test_ibm_qaoa_simulation_validation.py.

2. Question — does total_training_shots really include the recursion sub-depths?

a609fac replaces the INTERP training-cost proxy — N × M plus a hand-derived (p−1) × 50 × M correction — with total_training_shots × t_shot. The reasoning in the commit message is careful and the code guards on the column being present, so this is a question rather than a blocker. But it rests on a claim I cannot check from the repository, and it contradicts the comment it deletes.

The removed comment stated that stage 3's RecursionTrainer uses MPSAerEvaluator — exact, no .shots — so _augment_stage_result sets training_shots_used=0 and the sub-depths contribute nothing to total_training_shots. The new comment states the opposite. Both cannot be true, and which one holds decides whether INTERP training cost is now correct or silently understated by the entire recursion cost.

The reconciliation is probably this PR's own change at src/simulation_validation.py:1492-1494:

if "evaluator" in nested_init:
    nested_init["evaluator"] = "MPSAerSampleEvaluator"
    nested_init["evaluator_init"] = dict(evaluator_init)

That makes the sub-depths shot-based, so their shots would be counted — but only for campaigns generated after that change. If heavy_hex_144_I_p7_expanded predates it, its total_training_shots still excludes the sub-depths and the new formula understates INTERP cost.

Two things would settle it, and neither needs a campaign re-run:

• Post the actual numbers for a few INTERP rows — total_training_shots against N × M at the same (N, M, p). If sub-depths are counted, the ratio should track p, not sit at 1.
• Note in the notebook which campaign generation the data comes from, since results/ is not committed (only data/generated_instances is), so neither CI nor a reviewer can reproduce this.

Related: this is the behaviour you deferred under nested-recursion-evaluator-init, which now has a consumer. Worth folding into that follow-up rather than leaving both open.

3. Nonblocking — the T_proxy formula is now stated two ways

a609fac updated the methodology markdown (cell 4) to document INTERP using total_training_shots, but the section header further down (cell 8) still tells the reader:

All Window Sticker results below use T_proxy = t_preprocessing + N M t_shot + Q t_shot as the resource coordinate.

That is now wrong for INTERP, and cell 8 is the one a reader hits immediately before the figures. Please add the carve-out there, or point back to the cell 4 table. Same cell has a typo introduced in this commit: "a classical proxy to quatum hardware" → "quantum".

4. Nonblocking — the test plan in the PR body is stale

The body claims pytest tests/ -q — 360 passed and "27 new" tests. At this head it is 471 passed, 4 failed (the count moved with the base merge, and the colour revert/restore changed the new-test count). Worth refreshing once CI is green so the merge record matches.

5. Nonblocking — the unseeded bootstrap is now visible in committed output

Between 604befd and f3c36db the fitted resource model in the single-campaign cell moved from

Q(T) = exp(7.138 + 0.9703 log(T) + 0.02818 log(T)^2)

Q(T) = exp(7.063 + 0.9801 log(T) + 0.03174 log(T)^2)

with no code change touching that path — that campaign (heavy_hex_144_FA_no_opt_p6_expanded) contains only FA_PP_no_opt, so the INTERP change cannot explain it. This is the unseeded resampling tracked in #86 showing up directly in committed artifacts: re-running the notebook moves published coefficients. No action needed in this PR, but it is a concrete instance worth attaching to that issue, since it means figure refreshes are not reproducible.

Tests run and outcomes

At PR head f3c36db, in a clean copy of the head tree:

• pytest tests/ -q — 4 failed, 471 passed, reproducing CI exactly. All four failures are TestBuildBoundCircuitSimulator::* with ImportError: Sampling requires qiskit and qiskit-aer
• With the prescribed guard applied — 471 passed, 4 skipped
• Colour-map measurement and mutation probe as described above
• git merge-tree --write-tree upstream/main f3c36db — exit 0, no conflicts
• Notebook: 13 code cells, 0 error outputs, monotonic execution counts, 8 figures, 0 absolute paths in output
• Changed-file set matches GitHub exactly (15/15)

GitHub checks on f3c36db: test (3.11) and coverage-summary fail; test (3.10) and test (3.12) report cancelled from fail-fast, not independent failures; integration-tests reports skipped because its dependency failed, so it is a cascade rather than an intentional skip; tutorial-smoke passes.

I did not run a separate merge-result check: the branch now contains main, so the head is the merge result.

Summary

Blocking: 1 (inline) · Questions: 1 · Nonblocking: 3 · Total: 5

I would not merge this until the blocking issues above are addressed. Item 1 alone turns CI green and is independent of everything else — worth pushing on its own rather than batching it with the rest.

[bernalde]

Targeted re-review at 6b8e1fa, superseding my review at f3c36db. Everything I asked for last round is in, CI is green at this head, and the 18 commits since then hold up well except for one accounting defect in the new resume path that I'd like fixed before merge because it feeds the INTERP resource axis.

@anurag-r20 — one Blocking item (inline, run_fa_pss_exact_points), one question on the isotonic fit, and a handful of metadata/pin cleanups.

Resolved since f3c36db

• qiskit-aer guard — 01f19dc applies the class-level skipif exactly as prescribed; CI on 6b8e1fa collects 493 and reports 489 passed, 4 skipped on every lane, and I reproduce that locally. The inline thread is addressed and can be resolved.
• tproxy-doc-inconsistency — cell 8 now carries the INTERP carve-out and the "quantum" typo is fixed.
• The base merge, colour-collision fix, resource_match_bins docs (#85/#86), ZERO_TRAINING_METHODS dedup, dashed flat-hold and AUDIT_REPORT.md removal all still hold at this head. I re-ran the colour mutation: restoring the overlapping FA bands still reddens test_fa_star_and_fa_dagger_do_not_collide, and the new LR-tier guard behaves the same way (overlapping LR_star back onto LR reddens test_lr_star_and_lr_do_not_collide; dropping the LR_dagger branch reddens three tests). Worst cross-family colour distance over the 11 labels in the notebook is now 0.229.

New since f3c36db — what I reviewed

681e857 (per-grid-point resume), 18cdb90 (linear_ramp_no_opt in run_pt_pss_exact_points), 9fcee97 (isotonic prescription fit, LR tiers), 8c0a346/6b8e1fa (colorbar rows, dynamic Pareto legend), the Nautilus retry/pinning commits, three new LR job scripts, and the notebook re-executed on the I_full_p7 / LR campaigns. The isotonic replacement is guarded: disabling pooling in _pool_adjacent_violators reddens three tests, and dropping the Q-subset check in _cached_fa_grid_point reddens one. The resume path's cost accounting, however, has no test at all — zeroing the carried-over cumulative_shots leaves the suite green — and that is where the defect sits.

1. Blocking — resumed grid points double-count training cost (inline)

Details and fix on simulation_validation.py line 1962. Short version: after a cached (N, M) point the next computed point cold-starts with maxiter = n_value (full budget), but its evaluations, shots and wall-clock are added on top of the cached prefix. For n_values = [10, 20, 30], M = 100, an uninterrupted run reports total_training_shots = 3000 at N = 30; a run resumed after N = 20 reports 5000. training_cost_proxy is unaffected (it is N·M·t_shot analytically), so the exact and proxy columns disagree on resumed rows — and since a609fac made INTERP's T_proxy read total_training_shots directly, any resumed shard in heavy_hex_144_I_full_p7_expanded shifts the Interpolation curve right by the cached prefix.

2. Question — per-column isotonic prescriptions

9fcee97 replaces the degree-2 log-log polynomial with isotonic (PAVA) fits of N(T), M(T) and Q(T) separately. The motivation ("never recommend less as the budget grows") is right for Q, but N and M trade off inside the same budget: a recipe that shifts from few iterations at many shots to many iterations at fewer shots is a legitimate non-monotone M(T), and PAVA will flatten it into a plateau. The committed cell 9 output already shows M(T) collapsing to 73 knots. Is per-column monotonicity intended, or should the constraint be on the budget N·M (monotone product, free split)? Either answer is fine; the methodology cell should state it, and the two comments that still say "continuous polynomial fit" (simulation_validation.py:3579 and the snap_actionable_fit_to_feasible_grid call site) need updating.

3. Nonblocking — the PR body describes an older head

This is the merge record, so it is worth a pass before merge:

• "Stacked on #50 (base retargeted to QAOA_Parameter_Setting_IBM … 14 files)" — base is main, #50 is merged, the diff is 18 files.
• "pytest tests/ -q — 360 passed (27 new …)" — at this head CI and my local run give 489 passed, 4 skipped (493 collected); the test file has 31 tests. No commit in the branch ever had 27 tests in that file (69c5371 had 18).
• "FA_dagger moved off Blues onto Reds; also restored one legend handle per method family" — at this head both FA families are on Blues (0.55–0.92 / 0.15–0.42) and plot_multi_method_window_sticker_component_panels deliberately has no per-family handles (families are read off the colorbars). That is the right design; the body just describes the intermediate one.
• Not mentioned at all: the isotonic fit change, per-grid-point resume, the three LR tiers, linear_ramp_no_opt in run_pt_pss_exact_points, the LR p5/p9 and full-recursion campaigns, and the Nautilus retry/pinning work. Those are the changes a reader most needs.

4. Nonblocking — nested-recursion-evaluator-init deferral has no tracker

You deferred the evaluator_init overwrite / .setdefault() questions to a follow-up on 2026-08-15, and last round I asked to fold interp-training-cost-premise into the same follow-up. There is still no issue for it — #85 and #86 are the only open ones. Please open one (a few lines quoting the two questions is enough) and link it from the PR body, so the deferral survives the merge.

5. Nonblocking — INTERP campaign provenance

The notebook now sources Interpolation from heavy_hex_144_I_full_p7_expanded, generated by interp_full_recursion_job.sh pinned to this branch, which carries the MPSAerSampleEvaluator nested-evaluator change — so the sub-depth shots are counted for this data, and the printed multi-strategy table (Interp p=7 at 0.95 s versus 0.08 s for FA* p=7) is consistent with that. That answers last round's question well enough for me. Two lines in the methodology cell naming the campaign and the generator pins (qaoa_training_pipeline v0.1.0, QPS 50a17c6) would make it reproducible without reading the job script; and please confirm no shard of that campaign resumed from a checkpoint (see item 1), or say which rows did.

6. Nonblocking — container paths in committed output

Cell 5's test_specs_df table prints /workspace/data/generated_instances/… and /workspace/workdirs/lr-angle-opt-p5-finalize/… in graph_path/minmax_path. Not a contributor path this time, but the earlier scrubbing pass left the notebook at zero absolute paths and this run reintroduces some; the _relativize_paths helper could drop the /workspace/ prefix the same way it drops WORKSPACE_ROOT.

Tests run and outcomes

At PR head 6b8e1fa, in a clean worktree, repo .venv (Python 3.13, no qiskit-aer):

• pytest tests/ -q -rs — 489 passed, 4 skipped (the four TestBuildBoundCircuitSimulator cases, skip reason as prescribed); imports resolved to the worktree's examples/IBM_QAOA/src
• flake8 --select=F401,F841,E9,F63,F7,F82 on the four touched Python files — clean; bash -n on all changed shell scripts — clean
• Mutation probes: PAVA no-op → 3 failed; _cached_fa_grid_point subset check dropped → 1 failed; LR band overlap → 1 failed; LR_dagger branch removed → 3 failed; resume counter carry-over zeroed → 0 failed (item 1)
• Colorbar layout rendered synthetically with 7 families: 2 rows, uniform 0.202 width, titles per family
• Notebook at head: 13 code cells, execution counts 14–26 monotonic, 0 error outputs, 8 figures
• Changed-file set matches GitHub (18/18); merge-base equals main tip a83d145, so the head is the merge result

GitHub checks on 6b8e1fa (run 32594745628): test (3.10/3.11/3.12), tutorial-smoke, integration-tests, coverage-summary — all SUCCESS, none skipped. No sibling open PRs touch these files.

Summary

Blocking: 1 (inline) · Nonblocking: 7 (3 inline, 4 in this body) · Questions: 1 · Total: 9

I would not merge this until the blocking issues above are addressed. Item 1 is a small, local fix plus a test; once it lands and the body is refreshed I expect to approve.

[bernalde]

Verification pass at 8724c54: the four commits since my review land well — the resume double-count fix is correct at head, the power-law fit answers the isotonic question and is mutation-guarded, all fork/branch pins are fixed, the axis labels are generalised, the provenance note is in, and /workspace paths are gone. CI is green (494 passed, 4 skipped, matching my local run of the exact head tree). I've resolved the four threads that are done; the Blocking thread stays open for one remaining gap, and a few items have no thread, so listing everything still outstanding here.

@anurag-r20 — remaining before I can approve:

1. The resume fix's regression test doesn't actually guard it (open Blocking thread on simulation_validation.py). test_resumed_run_matches_uninterrupted_run_at_same_grid_point is vacuous: _simulate_grid_shots' cached branch never seeds cumulative_shots from the cached row, so the double-count it claims to reproduce cannot occur in it — it stays green with _reset_cumulative_cost_on_cold_start mutated to a no-op (only test_cold_start_resets_to_zero catches that), and deleting the production call to the helper leaves the entire suite green. Fix: in the mirror's cached branch, set cumulative_shots = n_value * m_value (what the cached row carries) before continue; that makes the scenario test fail under both mutations. Details in the thread.

2. PR body still describes an older head. It opens "Stacked on #50 (base retargeted to QAOA_Parameter_Setting_IBM … 14 files)" and claims "360 passed (27 new)" — at this head it's main-based, 18 files, 494 passed / 4 skipped (498 collected), and the colour paragraph describes the abandoned Reds/legend-handle design. It also doesn't mention the power-law fit, per-grid-point resume, LR tiers, linear_ramp_no_opt wiring, the LR/full-recursion campaigns, or the hardware-overlay recalibration. This is the merge record — please refresh it.

3. No tracking issue for the nested-recursion-evaluator-init deferral. You deferred the evaluator_init overwrite / .setdefault() questions to a follow-up on 2026-08-15; #85 and #86 are still the only open issues. Please open one and link it from the body so the deferral survives the merge.

4. Did any heavy_hex_144_I_full_p7_expanded shard resume from a checkpoint? The campaign was generated before the resume-accounting fix, so any resumed shard's total_training_shots — now the basis of INTERP's T_proxy — carries the inflated prefix. Please confirm no shard resumed, or name the affected rows (rows where total_training_shots exceeds N·M times the stage count are the tell).

5. Contributor path is back in committed output. Cell 7's stderr prints /mnt/c/Users/rames102/Desktop/stochastic-benchmark/src/interpolate.py:121: UserWarning: … — the warning formatter bypasses the print-scrubbing helpers. A warnings.formatwarning override (or filter) in the notebook prologue would keep the committed outputs path-free.

6. The hardware-overlay recalibration in 8724c54 is new since my review — I'll look at that cell together with item 1's test fix in the next pass rather than asking for changes now; if you can paste the numbers behind "QPU_time is ~0.3% of total for a representative point", that would speed it up.

Items 1–5 are small and well-defined. Once they're in I'll re-review, which is also what clears the standing changes-requested gate.

[anurag-r20]

Addressed items 1, 2, 3, 5 directly; answering 4 and 6 here.

1. Resume-fix regression test. Fixed per your exact suggestion: _simulate_grid_shots's cached-point branch now sets cumulative_shots = n_value * m_value before continue, so the reset call's effect is load-bearing. Mutation-tested both ways: deleting the production _reset_cumulative_cost_on_cold_start call now correctly fails the scenario test (5000 != 3000, the bug's exact signature), and test_cold_start_resets_to_zero already covers the helper mutated to a no-op directly.

2. PR body. Rewritten — correct base (main, 18 files), correct test counts, and it now covers the resume fix, power-law fit, LR tiers/linear_ramp_no_opt, the full-recursion campaigns, the hardware-overlay recalibration, and the warning-scrub fix. Corrected the stale colour paragraph too (Fixed Angles*/† both stay on Blues with disjoint bands, not the abandoned Reds design). Left one test-plan box honestly unchecked: Simulation_Method_Validation_and_WS.ipynb hasn't been re-executed end-to-end since today's plotting changes yet.

3. Tracking issue. Opened #87 and linked it from the PR body.

4. Did any heavy_hex_144_I_full_p7_expanded shard resume from a checkpoint? Yes — confirmed 10 of 210 (shard, instance, M) grid columns in the original campaign carried the inflated prefix from a resumed checkpoint. Rather than just naming the affected rows, I regenerated the entire campaign end-to-end with the fix applied (same instances, same grid) and verified the new data is clean: checked num_objective_evaluations sequences across all 30 instances × 343 grid columns (10,290 rows, matching the expected count exactly), zero rows showing the bug's inflated-offset signature.

5. Contributor path leak. Added a warnings.formatwarning override in the notebook's config cell, right after WORKSPACE_ROOT is defined — relativizes the warning's source filename the same way _relativize_paths already handles dataframe columns (relative to WORKSPACE_ROOT, or strips a container /workspace/ prefix). Verified standalone that it actually rewrites the path before commit.

6. Hardware-overlay numbers. For FA_PP_opt_6 (the strategy anchoring the top of the real-hardware frontier): raw QPU execution time averages 1.163 s across the 10 test instances, while dur_mean (QPU + classical training) is 404.99 s — QPU time is 0.287% of the total. Recalibrating QPU time onto the paper's shot rate (405 μs vs. the locally-derived ~271 μs) moves it from 1.163 s to 4.977 s (a real 4.3× shift, correctly tracking the calibration factor), but since that's still a tiny fraction of the total, dur_mean only moves from 404.99 s to 408.81 s — a 0.94% change, invisible on a log-scale axis spanning 4+ orders of magnitude. Classical training cost dominates the real-hardware budget for this campaign, not QPU sampling time.

[bernalde]

Final review at 2d033bc, superseding my review at 6b8e1fa. Every item from the previous round and the follow-up comment is addressed at this head, CI is green, and the two surfaces no round had judged yet — the CI-whisker/marker machinery in 93a778a and the hardware-overlay recalibration cell — both hold up. Approving, with three nonblocking notes for the record.

Verified since 6b8e1fa

• Resume double-count fix (32aa0d4) — correct at head. The mirror test is no longer vacuous: with _reset_cumulative_cost_on_cold_start mutated to a no-op, both test_cold_start_resets_to_zero and test_resumed_run_matches_uninterrupted_run_at_same_grid_point now fail (previously only the first did). See note 1 below for the one caveat.
• Power-law fit — slope-clip guard holds (removing max(slope, 0.0) reddens test_negative_trend_is_clipped_to_flat); stale "polynomial fit" comments updated.
• Pins (c4ee51e) — zero anurag-r20 / QAOA_Parameter_Setting_IBM / PR-branch references remain under nautilus/; lr_no_opt_job.sh uses the overridable ${STOCHASTIC_BENCHMARK_BRANCH:-main} form.
• Axis labels, provenance, warning scrub (c1058a8, 93a778a) — all four t_train labels in place; provenance note names the campaign and pins; _scrubbed_formatwarning verified working: the re-executed notebook has zero absolute paths in any output (was leaking /mnt/c/Users/... via the warning formatter), 0 error outputs, monotonic execution counts.
• Campaign regeneration — the committed notebook reflects the regenerated heavy_hex_144_I_full_p7_expanded data (Interp p=7 T_proxy moved 0.9519 → 0.9933 s). The 10/210-tainted-columns finding and the zero-inflated-rows check on the regenerated data are author-verified; results/ is not committed, so I could not reproduce them independently.
• CI-whisker/marker code (93a778a) — reviewed in full. _pareto_envelope_bounds reuses best_idx so whiskers always match the owning entry; whiskers are drawn only at segment starts, where the flat-hold subtlety cannot bite; the NaN-CI path is handled; the /1.96 rescale to 1 SEM is verified against the producer (response_lower/upper = response ± 1.96·SEM at utils.py:2857) and matches the hardware curve's dur_sem/ar_sem basis. The index-based unpack in _pareto_envelope_and_owner is backward-compatible (existing 4-tuple callers and tests pass).
• Hardware-overlay recalibration (8724c54 + cell 20) — the axis-basis fix is right: num_shots × t_calib(job_p) puts hardware QPU time on the same paper-measured shot-rate basis the simulated curves already use, while classical training wall-clock is correctly left unscaled. Your numbers (QPU 1.163 → 4.977 s, dur_mean 404.99 → 408.81 s, +0.94%) are internally consistent with that. The best-bitstring AR over the counts histogram matches this notebook's own BestApproximationRatio definition, and the committed cell rendered cleanly. One accepted methodology note: the hardware x-axis (training + QPU execution) and the simulated T_proxy (t_preprocessing + t_train + Q·t_shot) are analogous rather than identical resource definitions — fine for a validation overlay, worth a phrase in the paper text when this figure is used.

Nonblocking notes

1. The production call site of the reset helper is still unguarded

Deleting the _reset_cumulative_cost_on_cold_start call inside run_fa_pss_exact_points leaves the entire suite green — the mirror test exercises its own copy of the control flow, so it structurally cannot bind the production call (this also means the address comment's claim that this mutation "correctly fails the scenario test" doesn't hold; no such mirror-style test can). To be clear, the mirror-seeding fix is exactly what I prescribed, so the limitation is mine as much as anyone's. Closing it for real means a test that stubs the heavy calls (run_method_from_config, sample_bound_circuit_memory) and drives run_fa_pss_exact_points itself with a cached existing_df — reasonable to defer; a line in #87 or a small follow-up issue would keep it from being forgotten.

2. Two small inaccuracies in the rewritten PR body

• "+8824/−4922" — the PR now reports +8803/−4888 (the counts moved with the final re-exec commit).
• "one legend handle per method family alongside the per-family colorbars" — true of the older plot_multi_method_window_sticker_components figure, but the multi-strategy panels figure deliberately has none (utils.py:3749: families are read off the colorbars, which is the better design). Suggest "…; method families are identified by the per-family colorbars."

Fine to fix in a body edit without a new commit.

3. _pareto_envelope_bounds and the marker path have no tests; two stale comments in cell 20

The +123 lines in utils.py came with no unit tests (the test delta was the mirror seeding only). Two cheap ones would pin the contract: whiskers-match-owner (a two-entry crossover where the CI band must switch with best_idx) and the NaN-CI passthrough. Also, cell 20's prose says "One per calibration" and describes a "Noiseless-calibrated hardware curve", but the code builds and draws only the Noise-Corrected hardware frontier — trim the comment or add the second curve. Neither needs to gate this PR.

Tests run and outcomes

At PR head 2d033bc, in a byte-verified git archive copy, repo .venv (Python 3.13, no qiskit-aer):

• pytest tests/ -q — 494 passed, 4 skipped (498 collected), matching CI exactly; the 4 skips are the intended qiskit-aer guard
• Mutation probes: reset helper no-op → 2 failed (both resume tests); slope clip removed → 1 failed; production reset call deleted → 0 failed (note 1); source restored byte-clean after each
• Notebook audit: 0 error outputs, execution counts 1–14 monotonic, 0 absolute paths, provenance note present, cell 20 figure rendered and saved
• Changed-file set matches GitHub (18/18); merge base equals main tip a83d145, so the head is the merge result
• Issue #87 exists and cross-links both ways with the PR body

GitHub checks on 2d033bc (run 33447090785): test (3.10/3.11/3.12), tutorial-smoke, integration-tests, coverage-summary — all SUCCESS, none skipped. No other open PRs.

Summary

Blocking: 0 · Nonblocking: 3 (in this body) · Questions: 0 · Total: 3

Merge-ready. This approval supersedes my earlier requests for changes; the nonblocking notes above can land as a body edit and a line in #87, before or after merge at your discretion.