TenSolver Stochastic Benchmark example - #52
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a TenSolver stochastic-benchmark example dataset (Gurobi reference objectives + checkpoint CSVs) to support the Jupyter notebook benchmarking workflow described in the PR.
Changes:
- Added
gurobi_best.jsonreference objectives/status for QpLib instances 308–330. - Added performance and parameter checkpoint CSV artifacts used for plotting/projection.
- Added multiple “Projection from …” CSV outputs for training stats/results.
Reviewed changes
Copilot reviewed 7 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/TenSolver/results/gurobi_best.json | Adds Gurobi best-known objectives/status per instance for performance-ratio reference. |
| examples/TenSolver/checkpoints/performance_plotting/baseline.csv | Adds baseline curve data used for performance plotting. |
| examples/TenSolver/checkpoints/performance_plotting/Projection from TrainingStats.csv | Adds projected performance curve derived from training stats. |
| examples/TenSolver/checkpoints/performance_plotting/Projection from TrainingResults.csv | Adds projected performance curve derived from training results. |
| examples/TenSolver/checkpoints/params_plotting/baseline.csv | Adds baseline resource→(iteration,samples) mapping for parameter plotting. |
| examples/TenSolver/checkpoints/params_plotting/Projection from TrainingStats.csv | Adds projected resource→(iteration,samples) mapping from training stats. |
| examples/TenSolver/checkpoints/params_plotting/Projection from TrainingResults.csv | Adds projected resource→(iteration,samples) mapping from training results. |
removing files genereted by SB, it's not necessary to include those in the PR.
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues:
- The notebook is not CWD-independent. It fails when executed from the repository root because data files are opened through
results/...paths and the source path is added as../../src. - The benchmark is not reproducible because the bootstrap sampling and train/test split use NumPy randomness without a fixed seed; re-executing changed the committed displayed values.
Nonblocking issues:
- The notebook works around
run_baseline()by attaching a placeholder baseline object, which is brittle example code and should be replaced with a core/API fix or cleaner setup.
Questions:
- None.
Tests run and outcomes:
git diff --check upstream/main...HEAD: passed.- In-sandbox direct notebook code-cell smoke from repo root with the Python 3.10 CI environment: failed at cell 3 with
FileNotFoundError: results/energy_history.json. - In-sandbox direct notebook code-cell smoke from
examples/TenSolverwith the Python 3.10 CI environment: passed. PYTHONPATH=src MPLCONFIGDIR=/tmp/mpl-pr52 /home/bernalde/miniconda3/envs/stochastic-benchmark-ci-py310/bin/python -m pytest tests/ -v: 221 passed.jupyter nbconvert --executecould not be completed inside the sandbox because Jupyter kernels need local sockets; unsandboxed execution of an untrusted PR notebook was rejected, so I used the direct in-sandbox code-cell smoke instead.
I would not merge this until the blocking issues above are addressed.
| "import matplotlib.pyplot as plt\n", | ||
| "from matplotlib.colors import LogNorm, LinearSegmentedColormap\n", | ||
| "\n", | ||
| "with open(\"results/energy_history.json\") as f:\n", |
There was a problem hiding this comment.
Blocking: These data loads, together with sys.path.append("../../src") below, depend on the process CWD being examples/TenSolver. Running the notebook from the repository root fails before the first plot with FileNotFoundError: results/energy_history.json, which is a common way users and CI execute notebooks by path. Define a notebook base path once and use it for all data paths, sys.path, and stochastic_benchmark(..., here=...); for example, resolve HERE from either Path.cwd() or Path.cwd() / "examples" / "TenSolver", then open HERE / "results" / ....
There was a problem hiding this comment.
Addressed in e4fdf9f. The notebook now defines HERE, uses it for data paths, src imports, here=str(HERE), and plot outputs. Added tests/test_tensolver_notebook.py and verified full code-cell execution from the repo root.
There was a problem hiding this comment.
Addressed earlier in e4fdf9f. The notebook now defines HERE and uses it for data paths, source imports, here=str(HERE), and plot outputs. The repo-root path behavior is covered by tests/test_tensolver_notebook.py::test_first_tensolver_code_cell_runs_from_repo_root and the direct notebook code-cell smoke.
| "\n", | ||
| "sample_counts = [1, 10, 100, 1000]\n", | ||
| "\n", | ||
| "# Run bootstrap per instance with hardcoded baselines — avoids update_rule complexity\n", |
There was a problem hiding this comment.
Blocking: This benchmark has stochastic outputs but never seeds NumPy before bootstrapping or the later train/test split. Re-executing the notebook changed the committed output for instance 326; for example, the max PerfRatio iteration for 10 samples moved from 39 to 40 in my run. Set a fixed seed before the bootstrap loop and before sb.run_Stats(...), or use a local RNG if the framework supports one, then rerun the notebook outputs so the plots and conclusions are reproducible.
There was a problem hiding this comment.
Addressed in e4fdf9f. Added RANDOM_SEED and seeded NumPy before the bootstrap and sb.run_Stats(...) steps. Added a notebook seed smoke check in tests/test_tensolver_notebook.py; full notebook code-cell execution from the repo root passed.
There was a problem hiding this comment.
Addressed earlier in e4fdf9f. The notebook now defines RANDOM_SEED and seeds NumPy before both bootstrap sampling and sb.run_Stats(...). This is covered by tests/test_tensolver_notebook.py::test_tensolver_notebook_seeds_stochastic_steps; the direct notebook code-cell smoke also reproduced the seeded instance 326 table.
| "stParams = stats.StatsParameters(metrics=[\"PerfRatio\"], stats_measures=[stats.Median()])\n", | ||
| "sb.run_Stats(stParams, train_test_split)\n", | ||
| "\n", | ||
| "class _PlaceholderBaseline:\n", |
There was a problem hiding this comment.
Nonblocking: This placeholder baseline is a brittle workaround for run_baseline() calling get_experiment_parameters(), which expects self.baseline.recalibrate to already exist. Public examples should not teach users to attach dummy internal state. Prefer fixing stochastic_benchmark.run_baseline()/get_experiment_parameters() to construct the virtual-best baseline without a preexisting baseline, or add a small public setup helper, then remove this class from the notebook.
There was a problem hiding this comment.
Addressed in e4fdf9f. Removed the notebook placeholder and changed stochastic_benchmark.get_experiment_parameters() to use a no-op recalibrator until the first baseline exists. Added test_run_baseline_does_not_require_existing_baseline.
There was a problem hiding this comment.
|
Pushed commit:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues:
get_experiment_parameters()captures a permanent no-op recalibrator when called before a baseline exists. See the inline comment.
Nonblocking issues:
- None.
Questions:
- None.
Tests run and outcomes:
- Discovered test commands from README, TESTING.md, pyproject pytest config, Makefile, and GitHub Actions CI.
PYTHONPATH=src MPLCONFIGDIR=/tmp/mpl-pr52 /home/bernalde/miniconda3/envs/stochastic-benchmark-ci-py310/bin/python -m pytest tests/test_experiments.py::TestStochasticBenchmarkRuntimeErrors::test_run_baseline_does_not_require_existing_baseline tests/test_tensolver_notebook.py -v: 4 passed, 1 warning.- Direct in-sandbox execution of all TenSolver notebook code cells from the repository root with the Python 3.10 CI environment: passed and reproduced the seeded instance 326 table values.
PYTHONPATH=src MPLCONFIGDIR=/tmp/mpl-pr52 /home/bernalde/miniconda3/envs/stochastic-benchmark-ci-py310/bin/python -m pytest tests/ -v: 225 passed, 1 warning.git diff --check upstream/main...HEAD: passed.- GitHub CI on head
82a79009: test (3.10), test (3.11), test (3.12), integration-tests, and coverage-summary all succeeded.
I would not merge this until the blocking issues above are addressed.
| def get_experiment_parameters(self) -> ExperimentParameters: | ||
| baseline = getattr(self, "baseline", None) | ||
| baseline_recalibrate = ( | ||
| baseline.recalibrate if baseline is not None else lambda df: None |
There was a problem hiding this comment.
Blocking: This captures lambda df: None into every ExperimentParameters object created before self.baseline exists. That fixes the first run_baseline() call, but it also lets callers create projection/static/search experiments before run_baseline() and later call attach_runs() without recalibrating the now-existing baseline, because StaticRecommendationExperiment.attach_runs() calls the stale parent_params.baseline_recalibrate callback. That is a silent correctness regression for experiment ordering. Please make the callback resolve self.baseline at call time, for example by calling getattr(self, "baseline", None) inside baseline_recalibrate, or limit the no-op only to the ExperimentParameters used to construct VirtualBestBaseline. Add a regression test that creates experiment parameters before a baseline exists, attaches a baseline later, and verifies recalibration is invoked.
There was a problem hiding this comment.
Addressed in dc0a082. baseline_recalibrate now resolves self.baseline when invoked, so parameters created before a baseline exists do not keep a stale no-op. Added test_experiment_parameters_recalibrate_late_bound_baseline, which covers late baseline attachment through StaticRecommendationExperiment.attach_runs().
|
Pushed commit:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues:
- None.
Nonblocking issues:
- None.
Questions:
- None.
Tests run and outcomes:
- Discovered test commands from README, TESTING.md, pyproject pytest config, Makefile, and GitHub Actions CI.
PYTHONPATH=src MPLCONFIGDIR=/tmp/mpl-pr52 /home/bernalde/miniconda3/envs/stochastic-benchmark-ci-py310/bin/python -m pytest tests/test_experiments.py::TestStochasticBenchmarkRuntimeErrors::test_run_baseline_does_not_require_existing_baseline tests/test_experiments.py::TestStochasticBenchmarkRuntimeErrors::test_experiment_parameters_recalibrate_late_bound_baseline tests/test_tensolver_notebook.py -v: 5 passed, 1 warning.PYTHONPATH=src MPLCONFIGDIR=/tmp/mpl-pr52 /home/bernalde/miniconda3/envs/stochastic-benchmark-ci-py310/bin/python -m pytest tests/ -v: 226 passed, 1 warning.- Direct in-sandbox execution of all TenSolver notebook code cells from the repository root with the Python 3.10 CI environment: passed and reproduced the seeded instance 326 table.
git diff --check upstream/main...HEAD: passed.- GitHub CI on head
dc0a082:test (3.10),test (3.11),test (3.12),integration-tests, andcoverage-summarypassed.
Merge recommendation:
- The PR can be merged as-is.
TenSolver Benchmark Example
Adds a complete Jupyter notebook benchmarking TenSolver on 23 QUBO instances from the QpLib subset of QUBOLib (instances 308–330).
What's included
iterations × samplesused as a proxy for compute; interpolation, train/test split, virtual best baseline, and projection experiments follow the same pattern as the Simulated Annealing example.performance.png,iteration.png,samples.png.Data files
results/energy_history.jsonresults/random_baseline.jsonresults/gurobi_best.json