IBM QAOA parameter setting: simulation-validated Window Sticker analysis - #50
Conversation
- Expand IBM_QAOA patterns to all examples subdirectories - Add general artifact patterns (plots, checkpoints, progress) - Add data file patterns (pkl, npz, npy) - Ignore accidentally created repo root directory
Add comprehensive test fixtures for IBM QAOA data processing: - 4 real experimental JSON files with varied instance/depth configurations - 4 synthetic edge case fixtures (multi-trial, missing fields, empty) - README documenting schema, boundaries, and usage patterns Fixtures support testing of: - Single vs multi-trial bootstrap behavior - IBM-specific filename parsing - Missing field handling - Empty/malformed data edge cases
Add 32 tests organized in 6 test classes covering: Unit Tests: - QAOAResult dataclass creation - parse_qaoa_trial with various data formats - load_qaoa_results with missing/malformed data - convert_to_dataframe transformations - group_name_fcn filename parsing - prepare_stochastic_benchmark_data pickle I/O Integration Tests: - process_qaoa_data end-to-end pipeline - GTMinEnergy injection for missing ground truth - Single-trial bootstrap fabrication - Interpolation fallback behavior - Train/test split generation Edge Cases: - Missing trainer information - Missing optimal parameters - Empty trials list - Multi-trial synthetic data All tests use fixtures and proper mocking for multiprocessing. Test coverage validates IBM-specific logic boundaries.
Implement 4 high-impact optimizations for ~1K file scale: 1. ProcessingConfig dataclass for centralized configuration: - persist_raw: gate pickle writes during ingestion - interpolate_diversity_threshold: diversity-based interpolation - fabricate_single_trial: control single-trial bootstrap - seed: reproducible train/test splits - log_progress_interval: configurable progress logging 2. Structured logging infrastructure: - Replace print statements with logging module - Add progress logging every N files - Proper INFO/WARNING levels for errors - Timestamps and levels for production observability 3. In-memory aggregation with conditional pickle persistence: - persist_raw=True: write pickles to exp_raw/ subdirectory - persist_raw=False: aggregate in memory, skip ingestion pickles - Generate temporary pickles only when needed for bootstrap - Expected 1-2s savings for 1K files when disabled 4. Diversity-based interpolation heuristic: - Replace row count (n_rows <= 5) with diversity metric - diversity = unique_instances × unique_depths - Skip interpolation when diversity < threshold - Prevents spurious skips on sparse but valid grids Additional improvements: - Add try/except for malformed JSON files with warnings - Use config.seed for reproducible train/test splits - Fix pickle paths to use exp_raw subdirectory convention - Add enumeration to ingestion loop for progress tracking All changes maintain backward compatibility with default config. Expected performance improvement: ~15s for 1K files (from ~20-25s).
Add comprehensive performance optimization guide: Phase 1 - Implemented (4 changes): - ProcessingConfig dataclass for centralized configuration - Structured logging infrastructure - In-memory aggregation with persist_raw flag - Diversity-based interpolation heuristic Phase 2 - Deferred Enhancements (6 optimizations): 1. Parallel I/O with ThreadPoolExecutor (3-5x potential speedup) 2. Parquet output format (faster writes, smaller files) 3. orjson for JSON parsing (~2x speedup) 4. Lazy bootstrap fabrication (skip unnecessary computation) 5. Categorical dtypes for memory efficiency 6. Rich diversity metrics (entropy-based quality assessment) Each enhancement documented with: - Problem/solution description - Expected impact and thresholds - Implementation complexity - Testing requirements Target metrics: - Phase 1: <15s for 1K files (from ~20-25s baseline) - Phase 2: <10s with parallelization - Scale guidance: When to apply each optimization
Clear execution outputs and intermediate results to reduce repo size. Notebook structure and analysis code preserved.
Add ibm_qaoa_analysis_hardware.ipynb for analyzing real quantum hardware results from IBM systems. Complements simulation analysis with hardware- specific metrics and comparisons.
Move all pandas-specific test files from tests/Pandas_Group_Tests/ to tests/: - test_interpolate_pandas.py - test_stats_pandas.py - test_stochastic_benchmark_pandas.py - test_training_pandas.py Remove empty Pandas_Group_Tests subdirectory for better test organization. All 13 tests still passing after move.
- Update processing script to detect three optimization states: 'opt', 'noOpt', and None - Add marker differentiation in plotting: circles for opt, x for noOpt, squares for no flag - Use depth-specific colors for all marker types with appropriate legend labels - Extract optimization flag from filename patterns (_opt_, _noOpt_, or neither) - Fallback to Energy metric when Approximation Ratio not available in JSON
- Load minmax cuts from JSON files in R3R/minmax_cuts directory - Add maxcut_approximation_ratio() function using formula: cut_val = energy + 0.5 * sum_weights approx_ratio = (cut_val - min_cut) / (max_cut - min_cut) - Update convert_to_dataframe() to use calculated approximation ratios - Update process_qaoa_data() to load and pass minmax data - Add proper error handling and validation for edge cases
- Test minmax cuts loading from directory - Test approximation ratio calculation - Test end-to-end processing with minmax integration - Verify non-NaN approximation ratios in output
…d comparison - Update data loading to use 'optimized' column from process_qaoa_data - Create separate method names (FA_opt, FA_noOpt, TQA_opt, TQA_noOpt) - Update methods_to_compare list to include all 7 method variants - Change legends to single-column layout for better readability - Invalidate cache to force reprocessing with new method names - All variants treated independently in statistical analysis and rankings
- Fix regex false-match: 'PT' pattern matched 'pt' inside '_opt' (FA_PP_opt, LR_opt, FA_no_opt), causing runtime_train_wallclock (MPS wallclock, up to ~13 ks) to be added as t_preprocessing for all non-PT strategies, inflating T_proxy by ~3 orders of magnitude. Use alphanumeric-only boundary lookbehind/lookahead so 'PT' only matches standalone tokens. - Recompute T_proxy entirely from hardware-calibrated t_shot (≈0.28 ms, depth-independent for 144-qubit heavy-hex) instead of stored pickle values. - Add per-depth t_shot dict support in run_pt_pss_exact_points and run_fa_pss_exact_points via _resolve_time_per_shot helper. - Wire snap_actionable_fit_to_feasible_grid into run_stochastic_benchmark_pss pipeline with proper decoded-vs-raw recipe handling. - Fix latent pandas .copy() bug in src/training.py:285 (float scalar has no .copy()); fix resource_col hardcoding in setup_stochastic_benchmark_campaign. - Notebook: remove proxy-to-measured calibration cell and all dead imports/ config vars (EXACT_RESOURCE_ROOT, fit_exact_resource_models, etc.). - Multi-method plot: remove artificial flat-tail extrapolation that extended PT/FA-no-opt curves to the global x boundary; remove Actionable Pareto frontier overlay which was visually distracting. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces single-pod approach (which exceeded local memory) with 10-shard indexed job (3 instances each, 32Gi/64Gi per pod) plus separate finalize and cleanup jobs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
I_PP requires Julia + PauliPropagation.jl which is not available in the python:3.11-bookworm container; I_MPSAer uses MPSAerEvaluator (same Aer MPS backend used for sampling) with no additional dependencies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…paign build_sampled_training_config replaced top-level evaluators with MPSAerSampleEvaluator but left the RecursionTrainer's nested MPSAerEvaluator untouched. That evaluator defaults to chi=64, making I_MPSAer p=7 training ~10x slower than necessary. Now propagate chi from sample_config so the nested evaluator also uses chi=20. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Updates repos on PVC to latest commit and cleans previous failed output before shards are submitted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previous setup: IBM_QAOA_SKIP_REPO_UPDATE=1 + shared /workspace/repos pointed to usra-riacs upstream, which was missing our chi propagation fix and the I_MPSAer switch. Now each shard clones stochastic-benchmark from the user's fork (anurag-r20) into its own workdir/repos to guarantee correct code and avoid concurrent git conflicts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bernalde
left a comment
There was a problem hiding this comment.
Re-review at 3bf76ec. Thanks for the substantial rework — three of the four blockers from the earlier rounds are genuinely fixed, and the new tests/test_ibm_qaoa_processing.py is the right shape.
GitHub cannot render hunks for this PR (gh pr diff returns HTTP 406: diff exceeded the maximum number of lines (20000), and the pull-files endpoint only returns patch for the first 42 files — none of the files below). All findings are therefore in this body with stable markers rather than inline.
Status of the earlier threads at this head
tests/test_stochastic_benchmark_pandas.py/checkpoint_path: resolved. The fixture now feedsget_experiment_parameters(); all 7 tests pass.- IBM QAOA processing coverage: resolved.
tests/test_ibm_qaoa_processing.pyexists and its 8 tests pass against the currentsrc/modules. - Merge conflict with
main: resolved.mergeable: true, andgit merge-tree --write-tree origin/main 3bf76ecproduces a tree with no conflicts. - Personal absolute path default in
simulation_validation.py: partly resolved. The hardcoded/mnt/c/Users/...default is gone, but the replacement resolves to the wrong directory — see the first blocker. include_groups=False: not resolved, and I now have evidence it should not have been removed at all. Escalated to blocking below.- External-dependency setup question: partly answered. See the question at the end.
Blocking
Default external-repo paths resolve inside this checkout, not beside it
examples/IBM_QAOA/src/simulation_validation.py:32-33
DEFAULT_MAIN_REPO = Path(__file__).resolve().parents[3] / "QAOA-Parameter-Setting"
DEFAULT_PIPELINE_REPO = Path(__file__).resolve().parents[3] / "qaoa_training_pipeline"The file lives at <repo>/examples/IBM_QAOA/src/, so parents[3] is <repo> itself. The defaults therefore point at <repo>/QAOA-Parameter-Setting and <repo>/qaoa_training_pipeline — inside the stochastic-benchmark working tree.
Both new READMEs document the opposite layout:
<workspace>/stochastic-benchmark
<workspace>/QAOA-Parameter-Setting
<workspace>/qaoa_training_pipeline
(examples/IBM_QAOA/notebooks/README.md:43-50 "looks for sibling checkouts next to this repository", and examples/IBM_QAOA/src/README.md:34,49-55.) Anyone who follows the documented layout gets a FileNotFoundError on the default path and has to discover the env vars to recover, which is most of what the earlier thread asked to avoid. Use parents[4] for the two sibling repos; DEFAULT_INSTANCE_CACHE_ROOT on line 34 is correct as written (parents[1] → examples/IBM_QAOA/data/generated_instances, matching the README).
The four new notebooks are not registered in examples/tutorials.json
examples/tutorials.json is this repository's registry for example notebooks: scripts/verify_tutorials.py reads it, tests/test_verify_tutorials.py asserts on it, and the tutorial-smoke CI job runs from it. Every notebook under examples/ on main is registered — 7 of 7, verified by diffing git ls-tree -r --name-only main -- examples | grep '\.ipynb$' against the manifest paths.
This PR adds four and registers none:
examples/IBM_QAOA/notebooks/Analysis.ipynb
examples/IBM_QAOA/notebooks/Simulation_Method_Validation_and_WS.ipynb
examples/IBM_QAOA/Archive/ibm_qaoa_analysis.ipynb
examples/IBM_QAOA/Archive/ibm_qaoa_analysis_hardware.ipynb
verify_tutorials.py only walks the manifest, so nothing fails — the notebooks are simply invisible to the tutorial contract. The external category exists for exactly this case; please add entries with "category": "external" and a reason naming the required QAOA-Parameter-Setting / qaoa_training_pipeline checkouts (or drop the Archive/ notebooks entirely per the Archive finding below).
Analysis.ipynb ships a kernel-crash output on its final plot cell
examples/IBM_QAOA/notebooks/Analysis.ipynb
The PR description says "Run examples/IBM_QAOA/notebooks/Analysis.ipynb", "Notebook outputs are included for reference", and "Check generated plots for trends and performance". The committed outputs do not support that. Code cell index 28 — the recommendation plot, which is the notebook's headline result — carries an error output:
The Kernel crashed while executing code in the current cell or a previous cell.
That cell is build_recommendation_data → plot_ibm_qaoa_recommendation → print_budget_recommendations, and its execution_count is null, so it produced no figure and no budget table. The rest of the notebook is also not a single top-to-bottom run: execution_count goes 1, then 26 … 46, then null, null for the last two cells.
Please re-execute the notebook cleanly and commit outputs from that run, or if the crash is a resource limit on the recommendation cell, say so in the notebook and in the README and strip the error output rather than committing it. Simulation_Method_Validation_and_WS.ipynb has the same restart pattern (counts 275…281, 282, 289, last cell unexecuted) though no error output; worth regenerating together.
Nautilus manifests and runner default to this PR's own feature branch
Five places pin QAOA_Parameter_Setting_IBM:
examples/IBM_QAOA/nautilus/run_simulation_validation.sh:11examples/IBM_QAOA/nautilus/dev-pod.yaml:42examples/IBM_QAOA/nautilus/simulation-validation-job.yaml:109examples/IBM_QAOA/nautilus/simulation-validation-sharded-job.yaml:72examples/IBM_QAOA/nautilus/simulation-validation-finalize-job.yaml:62
Once this merges the branch is normally deleted, and git clone --branch "${STOCHASTIC_BENCHMARK_BRANCH}" under set -euo pipefail then fails at job startup — every manifest in the merged tree is broken on day one. Default STOCHASTIC_BENCHMARK_BRANCH to main in the script and in the four manifests (keeping the env-var override for branch testing), and update examples/IBM_QAOA/nautilus/README.md:28, which still instructs git push upstream QAOA_Parameter_Setting_IBM.
The token handling in these manifests looks right, for the record: secretKeyRef with optional: true, written to a 0600 credential file, never passed on a command line.
src/training.py drops include_groups=False, regressing main
src/training.py:141,153
) # include_groups=False # Pandas Version Errormain currently passes include_groups=False in both split_train_test calls; this PR removes them. The "Pandas Version Error" justification does not hold, and I checked it directly rather than reasoning from the changelog:
- pandas 2.3.3 (the floor in
requirements.txt,pandas>=2.3):include_groups=Falseis accepted and silent; omitting it emitsFutureWarning; the two results are.equals()-identical. - pandas 3.0.5: both forms are accepted and identical.
So removing it buys nothing and reintroduces the warning on the supported floor. It is also inconsistent within the same file — lines 111 and 328 still pass it, as do interpolate.py, stochastic_benchmark.py, and cross_validation.py, and tests/test_df_utils.py:127-150 asserts the kwarg is forwarded. Restoring it on both call sites keeps the suite green (I applied it locally: 332 passed at this head, tests/test_training.py 24 passed with FutureWarning promoted to visible). Please restore both, and drop the comment.
Four zero-byte test files are wired into run_tests.py unit
tests/test_interpolate_groups.py 0 bytes
tests/test_stats_groups.py 0 bytes
tests/test_stochastic_benchmark_groups.py 0 bytes
tests/test_training_groups.py 0 bytes
run_tests.py globs tests/test_*.py, so all four appear on the documented python run_tests.py unit command line and contribute zero tests. Whatever the "move Pandas group tests to main tests directory" commit intended, the group-variant content did not arrive. Either add the tests or delete the four files; an empty test_*.py advertises coverage that does not exist.
Nonblocking
examples/IBM_QAOA/Archive/ commits ~10k lines of non-working dead code
1.8 MB across nine files. It is not merely unused, it is broken and carries things that should not land:
flake8 --select=E9,F63,F7,F82reports 23 F821 errors, all inArchive/ibm_qaoa_processing.py:Dict(18) andAny(4) are never imported, and line 887 callsvalidate_edge_map, which does not exist anywhere in the file — the defined function isvalidate_edge_map_program_qubitsat line 715, with a different signature. The rest ofexamples/IBM_QAOAis clean (0 critical findings).Archive/transfer_hardware_data.py:32andArchive/transfer_simulation_data.py:224,226hardcode/mnt/c/Users/rames102/Desktop/QAOA-Parameter-Setting/...— the same class of path the earlier review asked to remove.Archive/test_approx_ratio_fix.pyis atest_*.pyfile;pytest examples/IBM_QAOA/Archivecollects 3 tests from it. It escapes CI only becausetestpaths = ["tests"].Archive/ibm_qaoa_analysis_hardware.ipynbcommits aNameError: name 'selected_p' is not definedoutput.
Git history already preserves this code. I'd drop the directory; if it must stay, at minimum remove the contributor paths, fix or delete the broken module, and rename the stray test file so it cannot be collected.
Committed notebook outputs still contain contributor absolute paths
examples/IBM_QAOA/notebooks/Simulation_Method_Validation_and_WS.ipynb prints /Users/rames102/Desktop/stochastic-benchmark/... and /Users/rames102/Desktop/QAOA-Parameter-Setting/data/hardware in its outputs (also truncated into several DataFrame HTML tables). The code defaults are now clean, so this is just stale output; it will go away with the re-execution asked for above.
tests/test_stats.py change is a removed trailing newline
The whole diff for this file is \ No newline at end of file on the last line. Please restore the newline and drop the file from the PR.
best_recommended behavior change is unrelated and untested
src/training.py:191 changes vb.groupby(resource_col).mean() to .mean(numeric_only=True). main's suite is green on pandas 3.0.5 without it (426 passed), so nothing in the repo demonstrates the problem it fixes, and no test covers the new behavior. It also changes the failure mode for a non-numeric parameter column from a TypeError at the aggregation to a KeyError at br[parameter_names + additional_cols]. Either split it into its own change with a test that fails before and passes after, or drop it from this PR.
flake8 findings in the new example code
flake8 examples/IBM_QAOA --exclude=examples/IBM_QAOA/Archive --select=F401,F841 reports 15. Two worth a look beyond tidiness:
src/utils.py:1561setsy_scale = 100.0and never uses it — in a plotting helper that computesmain_ymaxby hand just below, so it reads like an axis-scaling intent that was dropped.src/Processing.py:19-23importsstochastic_benchmark as SB,bootstrap,interpolate,stats, andfrom utils_ws import *, none used. The star-import in particular makes the module's namespace depend on the core package for no benefit.
The rest: Processing.py:12 (pickle), utils.py:11 (Patch), and unused locals at utils.py:991,1049,1142,1154,1216,1909.
tests/fixtures/ibm_qaoa/README.md points at a path that does not exist
Line 3 says the fixtures validate examples/IBM_QAOA/ibm_qaoa_processing.py. At this head that file is examples/IBM_QAOA/Archive/ibm_qaoa_processing.py, and the module the fixtures are actually exercised against is examples/IBM_QAOA/src/Processing.py (via tests/test_ibm_qaoa_processing.py). Please repoint it.
PR description understates the change
The body describes one notebook. The PR also adds a Nautilus/Kubernetes campaign harness, run_prepare_pss_campaign.py, a ~4.6k-line simulation_validation.py, a second notebook, ~600 KB of generated instance data, an Archive/ tree, and two edits to core src/training.py. It links no issue and no commit carries a closing keyword. Please bring the body up to date with the actual scope and the verification you ran, and add a Refs #... if there is a tracking issue — the description is what reviewers and post-merge readers work from.
src/__init__.py added with no explanation
An empty src/__init__.py turns the pythonpath = "src" directory into a package as well. Nothing in the PR appears to need it, and it makes import src.<mod> resolvable from the repo root at the same time tests/test_ibm_qaoa_processing.py relies on src resolving to examples/IBM_QAOA/src — the test only works because it inserts IBM_QAOA_ROOT at sys.path[0]. If it is not needed, drop it; if it is, say what needs it.
Pandas fixture stubs production objects with ad-hoc types
tests/test_stochastic_benchmark_pandas.py:32-33
sb.here = type('obj', (object,), {'checkpoints': str(tmp_path)})
sb.baseline = type('obj', (object,), {'recalibrate': lambda _df: None})()This satisfies get_experiment_parameters() and the tests pass, so the blocker is cleared. But here is really a names.paths(...) and baseline a VirtualBestBaseline, and the stubs are inconsistent with each other (here is assigned the class, baseline an instance). Using names.paths(tmp_path) for here would exercise the real object shape at no extra cost.
Questions
External dependency installation is still undocumented
The READMEs now name QAOA-Parameter-Setting and qaoa_training_pipeline and the three env vars, which answers most of the earlier question. Still missing is how to get the Python packages: requirements-examples.txt installs scikit-learn, dimod, nbconvert, ipykernel — no qiskit, no qiskit-aer. nautilus/run_simulation_validation.sh:81-85 shows the real recipe (pip install -e "${PIPELINE_REPO}" qiskit-aer, then pip install -e "${QPS_REPO}"). Should that become an examples/IBM_QAOA/requirements.txt, or a setup block in notebooks/README.md?
Should the generated instance data be committed?
examples/IBM_QAOA/data/generated_instances/heavy_hex__n=144__.../ adds 40 files (592 KB): 20 instance graphs at ~23 KB each plus 20 min/max-cut files. The code can regenerate these — generate_training_instances_like_qps and _compute_and_write_minmax — and the cache key is derived deterministically from graph type, node count, start index, and count. Is this committed deliberately as a reproducibility snapshot for the campaign, or is it scratch output that a user would regenerate locally? If it is deliberate, notebooks/README.md:27 is the place to say so, since it currently describes the directory as a cache.
Tests run and outcomes
Local .venv (Python 3.13.5, pandas 3.0.5, numpy 2.5.2, pytest 9.1.1), PR head 3bf76ec in a clean worktree:
pytest tests/ -q— 332 passed, 12 warnings.python run_tests.py unit— 321 passed; the command line includes the four zero-byte*_groups.pyfiles.flake8 src --select=E9,F63,F7,F82— 0.flake8 examples/IBM_QAOA --select=E9,F63,F7,F82— 23, all inArchive/ibm_qaoa_processing.py.flake8 examples/IBM_QAOA --exclude=Archive --select=E9,F63,F7,F82— 0;--select=F401,F841— 15.pytest examples/IBM_QAOA/Archive --collect-only— collects 3 tests fromArchive/test_approx_ratio_fix.py.- Probe: restoring
include_groups=Falseatsrc/training.py:141,153— 332 passed.
Merge result (origin/main b35f31d + 3bf76ec, temporary worktree): git merge clean, pytest tests/ -q — 447 passed. origin/main alone: 426 passed.
Baselines: pandas 2.3.3 and 3.0.5 probes for the include_groups finding, as described above.
GitHub checks on 3bf76ec: test (3.10), test (3.11), test (3.12), integration-tests, coverage-summary, tutorial-smoke — all pass. Note that none of them cover the two new notebooks, since they are absent from examples/tutorials.json.
Merge-readiness: the PR is mergeable and green, but it is 32 commits behind main and the diff (90 files, ~61k additions) exceeds GitHub's rendering limit, which is also why Copilot declined to review it. Splitting the Archive/ tree, the committed data, and the core src/training.py edits into separate changes would make the remaining analysis workflow reviewable.
I would not merge this until the blocking issues above are addressed.
…h defaults, training.py regression, dead code Blocking: - Fix DEFAULT_MAIN_REPO/DEFAULT_PIPELINE_REPO to use parents[4] instead of parents[3], which resolved inside this checkout instead of the documented sibling QAOA-Parameter-Setting/qaoa_training_pipeline layout. - Register Analysis.ipynb and Simulation_Method_Validation_and_WS.ipynb in examples/tutorials.json as external (require sibling checkouts + qiskit). - Default STOCHASTIC_BENCHMARK_BRANCH to main across all Nautilus manifests and run_simulation_validation.sh, so jobs don't break once this feature branch is deleted post-merge. Updated nautilus/README.md accordingly. - Restore include_groups=False in src/training.py (two call sites had it commented out under an incorrect "Pandas Version Error" justification; restoring it removes a FutureWarning under the required pandas floor). - Delete four zero-byte tests/test_*_groups.py files that were wired into run_tests.py unit but contributed no coverage. Nonblocking: - Delete examples/IBM_QAOA/Archive/ (confirmed broken: flake8 F821s, a call to a function that doesn't exist, hardcoded personal paths, a stray test_*.py file that leaked into pytest collection). - Revert src/training.py's best_recommended to numeric_only=True (verified against a real crash: virtual-best frames carry non-numeric columns, and pandas 3.x's string dtype makes plain .mean() raise TypeError instead of ignoring them). Added a regression test that fails without the fix. - Clear all flake8 F401/F841 findings in examples/IBM_QAOA (unused imports in Processing.py, unused locals/imports in utils.py). - Fix stale path in tests/fixtures/ibm_qaoa/README.md. - Keep examples/IBM_QAOA/src/__init__.py, verified empirically that removing it breaks tests/test_ibm_qaoa_processing.py collection (pyproject.toml's pythonpath = "src" makes it ambiguous with the core src/ package without it); documented why in the file. - Restore missing trailing newline in tests/test_stats.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bernalde
left a comment
There was a problem hiding this comment.
Re-review at 3f2d783 (9 new commits since my review at 3bf76ec). This is a strong round — 11 of the 15 findings from that review are fixed, and I verified each one rather than taking the commit message for it. One new problem came in with the p=7 sharded manifests, and four earlier items are still open.
As before, GitHub cannot render this diff (gh pr diff → HTTP 406: diff exceeded the maximum number of lines (20000); only 37 of the 82 files return a patch, and none of the ones below). Findings are in this body with stable markers rather than inline.
Resolved since 3bf76ec
| Prior finding | Status | How I checked |
|---|---|---|
| Default external-repo paths | Fixed | parents[4] now resolves to <workspace>/QAOA-Parameter-Setting, matching both READMEs |
| Tutorials manifest not updated | Fixed | All 9 notebooks in the tree are registered; scripts/verify_tutorials.py --dry-run prints SKIP … (external) with the reason for both IBM_QAOA notebooks |
Analysis.ipynb kernel-crash output |
Fixed | Both notebooks: 0 error outputs, 0 unexecuted cells, monotonic execution_count (2–24 and 1–8) |
| Nautilus feature-branch pins | Fixed (branch half) | Every STOCHASTIC_BENCHMARK_BRANCH now defaults to main; run_simulation_validation.sh:11 too. See the new finding below for the repo-URL half |
include_groups=False regression |
Fixed | Restored at src/training.py:141,154; all four call sites in the file now agree |
Four zero-byte *_groups.py test files |
Fixed | Deleted |
Archive/ dead code |
Fixed | Directory removed; flake8 examples/IBM_QAOA --select=E9,F63,F7,F82 is now 0 (was 23) |
tests/test_stats.py trailing newline |
Fixed | File ends in \n again |
best_recommended numeric-only untested |
Fixed | test_best_recommended_ignores_non_numeric_columns added. I mutation-tested it: reverting line 195 to .mean() makes it fail with exactly TypeError: dtype 'str' does not support operation 'mean', so it genuinely guards the change |
| flake8 unused imports/locals in new code | Fixed | --select=F401,F841 on examples/IBM_QAOA is now 0 (was 15) |
tests/fixtures/ibm_qaoa/README.md stale path |
Fixed | Repointed to examples/IBM_QAOA/src/Processing.py |
Blocking
Two p=7 manifests clone from a personal fork instead of this repository
The branch-pin half of my earlier Nautilus finding is fixed, but commit 02404ff ("Switch I_MPSAer p=7 shards to clone from user fork with per-shard repos") introduced the same problem one field over. STOCHASTIC_BENCHMARK_REPO across the manifests:
dev-pod.yaml usra-riacs/stochastic-benchmark.git
simulation-validation-finalize-job.yaml usra-riacs/stochastic-benchmark.git
simulation-validation-i-pp-p7-finalize-job.yaml anurag-r20/stochastic-benchmark.git <-- line 67
simulation-validation-i-pp-p7-job.yaml usra-riacs/stochastic-benchmark.git
simulation-validation-i-pp-p7-setup-job.yaml usra-riacs/stochastic-benchmark.git
simulation-validation-i-pp-p7-sharded-job.yaml anurag-r20/stochastic-benchmark.git <-- line 76
simulation-validation-job.yaml usra-riacs/stochastic-benchmark.git
simulation-validation-sharded-job.yaml usra-riacs/stochastic-benchmark.git
run_simulation_validation.sh:10 also defaults to usra-riacs. Both outliers now sit next to STOCHASTIC_BENCHMARK_BRANCH: main, so once this merges those two jobs clone main from a contributor's personal fork — not from this repository. The campaign then runs whatever that fork's main happens to contain, and breaks outright if it is renamed, made private, or deleted. Nothing in CI can catch it.
Please point both at https://github.com/usra-riacs/stochastic-benchmark.git, matching the other six. If per-shard cloning genuinely needed a fork during development, the env var already overrides it at apply time without committing the fork as the default.
Nonblocking
Contributor absolute paths are still committed — now also in the data files
Third round for this one, and it is wider than I scoped it last time. /mnt/c/Users/rames102/… appears on 123 lines across 22 committed files:
-
examples/IBM_QAOA/notebooks/Analysis.ipynb(2 lines) andSimulation_Method_Validation_and_WS.ipynb(81 lines) — still in the outputs after the re-execution, since it ran on the same machine. -
20 data files under
data/generated_instances/…/minmax_cuts/heavy_hex/*.json, each carrying an"instance"field like:/mnt/c/Users/rames102/Desktop/stochastic-benchmark/examples/IBM_QAOA/results/pss_window_sticker/heavy_hex_144_FA_PP_opt_p5_expanded/generated_instances/instances/heavy_hex/100_7_3_heavyhex_144nodes_weighted.json
The producer is examples/IBM_QAOA/src/simulation_validation.py:681, "instance": str(graph_path). I checked whether anything consumes it: the only reader of these files is examples/IBM_QAOA/src/approx_ratio_calc.py:81, which requires min_cut, max_cut and sum_of_weights and never touches instance. So this is provenance noise rather than a functional break — which is why it stays Nonblocking — but it permanently records one contributor's home directory layout in the repository.
Writing a repo-relative path (or spec.instance_id) at line 681 and rewriting the 20 committed files is a small, self-contained change.
PR description still describes roughly 2% of the change
Unchanged since the first round. The body is four lines about running Analysis.ipynb; the PR is 82 files and 45,683 insertions, including the Nautilus/Kubernetes campaign harness, a ~4.6k-line simulation_validation.py, run_prepare_pss_campaign.py, a second notebook, ~590 KB of committed instance data, and two edits to core src/training.py. There is still no linked issue and no closing keyword. (It also reads "This PR adds adds…".)
This is the merge record for a 45k-line change — please bring it up to date with the actual scope and the verification you have run. The rest of the review round is genuinely good work that the description gives no credit for.
Root src/__init__.py is still unexplained
Half-addressed, and the half that landed is the right pattern: examples/IBM_QAOA/src/__init__.py now carries a four-line comment explaining exactly why it exists (keeping that src a regular package so it does not merge with the core src/ into an ambiguous namespace package). The repo-root src/__init__.py is still a 0-byte file with no such note. Either give it the same one-line rationale or drop it.
Pandas fixture still stubs production objects with ad-hoc types
tests/test_stochastic_benchmark_pandas.py:32-33 is unchanged:
sb.here = type('obj', (object,), {'checkpoints': str(tmp_path)})
sb.baseline = type('obj', (object,), {'recalibrate': lambda _df: None})()Still inconsistent with each other — here is assigned the class, baseline an instance — and here is really a names.paths(...). Using names.paths(tmp_path) exercises the real object shape at no extra cost.
Coordinate the landing order with #84
#84 is branched from this PR's tip and currently targets main, so it carries all 80 of this PR's commits plus 2 of its own. I have asked there to retarget its base onto QAOA_Parameter_Setting_IBM or wait for this to land. Worth knowing here because the order matters: if #84 merges first it silently lands this PR without its review finishing, and if this PR gets new commits (for the blocker above), #84 needs a rebase before its own diff is meaningful. Landing this one first is the cleaner sequence.
Questions
External Python dependencies are still undocumented
Unchanged from last round. No qiskit or qiskit-aer in any requirements*.txt, no examples/IBM_QAOA/requirements.txt, and notebooks/README.md only links the two repositories without saying how to install them. The working recipe is still only visible in nautilus/run_simulation_validation.sh:81-85 (pip install -e "${PIPELINE_REPO}" qiskit-aer, then pip install -e "${QPS_REPO}"). Should that become an examples/IBM_QAOA/requirements.txt, or a setup block in notebooks/README.md?
Is the committed instance data a deliberate snapshot?
Also unchanged. notebooks/README.md:27 still describes data/generated_instances as a cache ("cached under ../data/generated_instances by default"), which reads as regenerable scratch, while 40 files (~590 KB) are committed. If they are a deliberate reproducibility snapshot for the campaign, that line is the place to say so; if they are regenerable, they probably should not be in the tree.
Tests run and outcomes
Local .venv (Python 3.13.5, pandas 3.0.5, numpy 2.5.2), clean worktrees.
At PR head 3f2d783:
pytest tests/ -q— 333 passed, 12 warningspython run_tests.py unit— passedflake8 src --select=E9,F63,F7,F82— 0flake8 examples/IBM_QAOA --select=E9,F63,F7,F82— 0 (was 23 at3bf76ec)flake8 examples/IBM_QAOA --select=F401,F841— 0 (was 15)scripts/verify_tutorials.py --dry-run— both new notebooks correctly skipped asexternalwith reasons- Mutation probe: reverting
numeric_only=Trueatsrc/training.py:195turns the new test red (TypeError: dtype 'str' does not support operation 'mean'), then restored. Failing there is the intended result — it confirms the new test actually guards the fix rather than passing incidentally.
Merge result (origin/main b35f31d + 3f2d783, temporary worktree): git merge clean, git merge-tree --write-tree reports no conflicts, pytest tests/ -q — 448 passed. origin/main alone — 426 passed.
GitHub checks on 3f2d783: test (3.10), test (3.11), test (3.12), tutorial-smoke, integration-tests, coverage-summary — 6/6 pass, none skipped. The two new notebooks are registered external, so tutorial-smoke deliberately skips executing them.
Summary
Blocking: 1 · Nonblocking: 5 · Questions: 2 · Total: 8 (all in this body; inline anchoring is unavailable on this diff)
The PR is mergeable with no conflicts and green on all six checks, and it is still 32 commits behind main. The one blocker is a two-line fix.
I would not merge this until the blocking issues above are addressed.
Blocking: - Point the two p=7 Nautilus manifests (simulation-validation-i-pp-p7-finalize-job.yaml, simulation-validation-i-pp-p7-sharded-job.yaml) back at usra-riacs/stochastic-benchmark.git instead of a contributor's personal fork, matching the other six manifests. Nonblocking: - Stop writing a contributor absolute path into the committed minmax-cut JSON files' "instance" field; write just the graph filename, which is all any consumer reads. Rewrote the 20 already-committed files. - Scrub contributor absolute paths out of the committed notebook outputs in Simulation_Method_Validation_and_WS.ipynb, and fix the print/display code so future runs report repo-relative paths instead of resolved absolute ones. - Drop the empty root src/__init__.py: verified (full test suite green without it) that nothing in the PR actually needs it, unlike the examples/IBM_QAOA/src/__init__.py counterpart which is load-bearing. - Use the real names.paths(tmp_path) object for the stochastic_benchmark test fixture's `here` attribute instead of an ad-hoc anonymous type, so it exercises the same object shape as production and stays consistent with the `baseline` stub already being an instance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add qiskit-aer==0.17.2 to requirements-examples.txt and document the two sibling-repo editable installs in notebooks/README.md, mirroring the working recipe in nautilus/run_simulation_validation.sh. - Clarify that the committed heavy-hex n=144 instance/min-max-cut snapshot is a deliberate reproducibility pin for the campaigns in this PR, not regenerable scratch output, and reword the "cache" language accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fixes Re-executed end-to-end so the four cells that call plot_multi_method_window_sticker_component_panels and plot_pareto_frontier_overlay show the current Reds FA_dagger colormap, restored per-family legend handles, and dashed extend_curves_to_xlim segments, instead of stale pre-fix images. Also carries over the print/display path-scrubbing fix from PR #50 (this branch predates that commit, so re-executing without it reintroduced contributor absolute paths in the output). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same bug as the one hit on the follow-up PR: test_specs_df's graph_path/ minmax_path can point directly into the sibling QAOA-Parameter-Setting checkout, not just into stochastic-benchmark's own data/generated_instances cache, and the path-scrubbing helper only handled paths under REPO_ROOT -- raising ValueError for any row loaded from the sibling repo. Use WORKSPACE_ROOT (the shared parent of both sibling checkouts) as the base instead, and fall back to leaving a path unscrubbed rather than crashing if it's ever outside even that. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same issue caught on the follow-up PR: save_current_plot's "Saved: ..." print used the raw (absolute) plot_dir. Prints relative to cwd via os.path.relpath now, which never raises (unlike Path.relative_to), so it degrades gracefully if cwd and plot_dir are ever on different filesystem roots. No currently-committed notebook output was affected (both call this with a relative plot_dir today), but it's a live bug in the shared helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tive paths The two fallback defaults for the QAOA-Parameter-Setting checkout hardcoded a contributor's home directory. Both now resolve to the sibling layout the READMEs document (<workspace>/QAOA-Parameter-Setting), matching the parents[4] convention already used in src/simulation_validation.py. Source-only change; the notebook's committed outputs never referenced these paths and are unmodified.
bernalde
left a comment
There was a problem hiding this comment.
Approving at 991c8e9. This supersedes my CHANGES_REQUESTED review at 3f2d783 — every finding from that round is now verified fixed at this head.
Disclosure: the final commit 991c8e9 is mine (two lines, replacing the contributor-path fallbacks in Analysis.ipynb), so I am approving a PR whose last commit I wrote. It is a two-line change in an 82-file PR and the rest is Anurag's work, but it should be on the record.
Findings from the last round, at this head
| Finding | Status |
|---|---|
nautilus-personal-fork-url (Blocking) |
Fixed — all 8 manifests use usra-riacs/stochastic-benchmark.git; zero fork references tree-wide |
contributor-paths-in-committed-artifacts |
Fixed — /mnt/c/Users/… now returns 0 hits anywhere in the tree. 4397260 scrubbed the 20 min/max-cut data files and changed the producer to Path(graph_path).name; d912fb7 stopped save_current_plot printing an absolute path; 991c8e9 replaced the two Analysis.ipynb fallbacks with the documented sibling layout |
pr-description-understates-scope |
Fixed — body rewritten to the real scope, and every claim in its test plan reproduces (see below) |
root-src-init-py-unexplained |
Fixed — src/__init__.py removed |
pandas-fixture-stubs-adhoc-types |
Fixed — sb.here = names.paths(str(tmp_path)) |
pr-84-stacking-coordination |
Fixed — #84 retargeted to QAOA_Parameter_Setting_IBM |
example-dependency-setup (Question) |
Answered — qiskit-aer==0.17.2 in requirements-examples.txt, plus a setup section in notebooks/README.md |
committed-generated-instance-data (Question) |
Answered — notebooks/README.md now states the snapshot is deliberate |
Two commits since that round were unprompted fixes rather than review responses, and both check out:
a7cd6f1—_relativize_pathsraisedValueErrorfor any spec row whosegraph_path/minmax_pathpointed into the siblingQAOA-Parameter-Settingcheckout rather than underREPO_ROOT. The fix rebases onWORKSPACE_ROOT(defined asREPO_ROOT.parentin the same notebook) and falls back to leaving a path unscrubbed instead of crashing. Correct and appropriately defensive for what is a display convenience.d912fb7—save_current_plotnow prints a cwd-relative path when given an absoluteplot_dir.
Tests run and outcomes
At PR head 991c8e9 (clean worktree, Python 3.13.5 / pandas 3.0.5 / numpy 2.5.2):
pytest tests/ -q— 333 passedpython run_tests.py unit— passedflake8 src --select=E9,F63,F7,F82— 0flake8 --select=E9,F63,F7,F82,F401,F841 examples/IBM_QAOA— 0scripts/verify_tutorials.py --dry-run— all 9 notebooks resolve; both IBM_QAOA notebooks correctlySKIP … (external)with reasons- Both notebooks: 0 error outputs, 0 unexecuted cells, monotonic execution counts, 0 absolute paths in any output
- Changed-file set matches GitHub's exactly (82/82)
Every line of the PR body's test plan reproduces.
Merge result (origin/main b35f31d + 991c8e9, temporary worktree): merge is clean, pytest tests/ -q — 448 passed. origin/main alone — 426 passed.
GitHub checks on 991c8e9: test (3.10), test (3.11), test (3.12), tutorial-smoke, integration-tests, coverage-summary — 6/6 pass, none skipped.
Merge readiness
No blocking issues, no nonblocking issues, no open questions — 0 findings in this review, and 0 unresolved review threads.
The branch is 32 commits behind main, which is why the merge-result run above matters more than the head run; both are green.
One coordination note for after the merge: #84 targets QAOA_Parameter_Setting_IBM, so leaving that branch in place keeps #84 stacked and collapses its diff to its own 15 files. #84 is not ready independently — it still needs a base merge to clear a notebook conflict and a pytest.importorskip("qiskit_aer") guard on TestBuildBoundCircuitSimulator, whose four tests fail in any environment without qiskit installed.
# Conflicts: # examples/IBM_QAOA/notebooks/Simulation_Method_Validation_and_WS.ipynb
Summary
Adds an end-to-end QAOA parameter-setting analysis workflow built on IBM
Quantum hardware and simulation data, plus the campaign infrastructure that
produces its inputs. 82 files, ~45.7k insertions. Main pieces:
Analysis.ipynb: loads IBM hardware + training data, computes MaxCutapproximation ratios, builds resource-cost columns (QPU time + estimated
training time), and produces performance/training-cost plots.
Simulation_Method_Validation_and_WS.ipynb: simulation-methodvalidation and Window Sticker notebook for the FA/PT-style PSS campaign,
loading frontier data from a campaign run.
src/simulation_validation.py(~4.6k lines): the PSS campaignmachinery — instance generation, exact-point sweeps with checkpointing,
Aer-backed simulation (statevector and MPS), and the resource frontier
construction consumed by the notebook.
src/Processing.py/src/approx_ratio_calc.py/src/utils.py:hardware/training data loading, approximation-ratio and min/max-cut
helpers, and the plotting/reporting helpers used by both notebooks.
run_prepare_pss_campaign.py/run_job.sh: CLI entry points forrunning PSS campaigns outside the notebook.
nautilus/): job specs for running thesimulation-validation campaign on a Kubernetes cluster (dev pod, single
job, sharded job, finalize job).
instances and their min/max-cut values (
data/generated_instances/), andtwo small, targeted edits to core
src/training.py(see below).Two edits outside
examples/IBM_QAOA/:src/training.py:best_recommendednow passesnumeric_only=Trueto.groupby().mean(), since virtual-best frames here carry non-numericcolumns and pandas 3.x's string dtype otherwise raises
TypeError(regression test:
test_best_recommended_ignores_non_numeric_columns).examples/tutorials.jsonasexternal(they require the sibling checkouts below).External dependencies
Simulation_Method_Validation_and_WS.ipynbandrun_prepare_pss_campaign.pyrequire two sibling repository checkouts (
QAOA-Parameter-Setting,qaoa_training_pipeline) plusqiskit-aer. Setup instructions, env vars,and the deliberate-snapshot rationale for the committed instance data are in
examples/IBM_QAOA/notebooks/README.md.How to run
Analysis.ipynb: run top to bottom; outputs are committed for reference.Simulation_Method_Validation_and_WS.ipynb: requires a campaign alreadyrun via
run_job.sh/run_prepare_pss_campaign.py(or the Nautilusmanifests) into
examples/IBM_QAOA/results/.Test plan
pytest tests/ -q— 333 passedpython run_tests.py unit— passedflake8 --select=E9,F63,F7,F82,F401,F841onexamples/IBM_QAOA— cleanscripts/verify_tutorials.py --dry-run— both new notebooks correctlyskipped as
externalexecution counts