Add README JuMP smoke test#22
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Review as repository maintainer.
I am the PR author, so GitHub does not allow me to submit a formal APPROVE or REQUEST_CHANGES on this PR (HTTP 422); this is submitted as COMMENT. The formal verdict must come from another maintainer. Based on my review there are no blocking issues and the PR is merge-ready.
What the change does: adds a README JuMP workflow testset that builds the exact 3-variable binary QUBO from the README with PySA.Optimizer, sets MOI.Silent() plus reads/sweeps/replicas/temperature attributes, optimizes, and asserts a result exists, the objective is finite, and the returned values are binary. Adds JuMP as a direct test dependency.
Verification:
- Diff confirmed against merge-base c34452e: 2 files, +31/-0, matching the PR's reported totals.
- All five solver attributes used in the test exist in
src/PySA.jl:24-28(NumberOfReads,NumberOfSweeps,NumberOfReplicas,MinimumTemperature,MaximumTemperature), andPySA.X()access matches howsrc/PySA.jlitself references them (e.g. line 63). - The test mirrors the README example (
README.md:21-37): sameQ,n = 3, binaryx, andMin x' * Q * xobjective. It genuinely covers the documented workflow. - Adding
JuMPtotest/Project.tomlwithout a[compat]entry is consistent with repo convention (no test dep has one). - CI is green on all four jobs (ubuntu + windows, Julia 1.10 and 1). Those jobs run the full
Pkg.test(), which now includes this testset and the conda install of pysa, so the behavioral claim is confirmed on both Linux and Windows.
Intent vs issue #19: the issue asks for a compact package-owned JuMP/QUBO smoke test exercising reads/sweeps/replicas/temperature, kept robust to SA randomness, asserting at least one result and that objective/value queries work, with Pkg.test() passing locally and in CI. The PR delivers all of this. The randomness-robust assertions (result count, finiteness, binary-ness rather than an exact optimum) satisfy the determinism criterion. Closes #19 is the correct link (full resolution).
-
Blocking issues: none.
-
Nonblocking issues:
- The test sets the temperature bounds and other attributes but never asserts the solve found a sensible solution. For this
Qthe unique optimum is -1 (a single variable set to 1; all-zero gives 0, two-ones gives 2). The assertions would pass even if the solver returned the all-zero or any feasible point, so the test guards that the pipeline runs but not that it solves. Optional: assertobjective_value <= 0(a weak, randomness-safe correctness signal) or, given the tiny problem and the configured reads/replicas, the exact optimum. Acknowledged tradeoff against flakiness; leaving it as-is is defensible for a smoke test. - The test mixes
JuMP.set_attribute(forMOI.Silent()) andJuMP.set_optimizer_attribute(for the solver attributes) for the same kind of operation. Using one consistently would read better;set_attributeis the current unified JuMP API. Minor.
-
Questions: none.
-
Tests run and outcomes:
- Statically verified the attributes, API access pattern, and README correspondence against the head tree.
- Did not run
Pkg.test()locally: it requires a network conda/pip install of upstream pysa and the built Python environment. Relied on the PR's CI, which runs the full suite (including this testset) on ubuntu and windows for Julia 1.10 and 1 — all four jobs pass.
- Merge-readiness: merge-ready. Test-only change, well-scoped, mirrors the documented workflow, robust to SA randomness, and CI passes on all supported platforms. The nonblocking items are optional improvements.
| JuMP.optimize!(model) | ||
|
|
||
| @test JuMP.result_count(model) >= 1 | ||
| @test isfinite(JuMP.objective_value(model; result = 1)) |
There was a problem hiding this comment.
Nonblocking (optional): this asserts the objective is finite but not that the solve actually minimized anything. For this Q the optimum is -1 (single variable set; all-zero gives 0, two-ones gives 2), so the test would pass on a non-optimal or even all-zero point. Consider a randomness-safe correctness signal such as @test JuMP.objective_value(model; result = 1) <= 0. A stricter == -1 would more truly guard the README workflow but risks flakiness under simulated-annealing randomness, so this is a judgment call.
There was a problem hiding this comment.
Addressed in f9f699a: the test now stores the first result objective, keeps the finite check, and adds @test objective <= 0. This rejects the all-zero outcome without requiring the exact optimum. Verified with Pkg.test(); the README JuMP workflow is now 5/5.
| @testset "README JuMP workflow" begin | ||
| model = JuMP.Model(PySA.Optimizer) | ||
| JuMP.set_attribute(model, MOI.Silent(), true) | ||
| JuMP.set_optimizer_attribute(model, PySA.NumberOfReads(), 4) |
There was a problem hiding this comment.
Nonblocking (minor): line 33 uses JuMP.set_attribute while lines 34-38 use JuMP.set_optimizer_attribute for the same kind of operation. Using one consistently (set_attribute is the current unified JuMP API) reads better.
There was a problem hiding this comment.
Addressed in f9f699a: the PySA solver attributes now use JuMP.set_attribute consistently with MOI.Silent(). Verified with Pkg.test().
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues: none.
Nonblocking issues: none.
Questions: none.
Tests run and outcomes:
- Verified PR metadata against the local merge-base diff: 2 files changed, 31 insertions, 0 deletions.
- Read issue #19 and confirmed this PR covers the requested README-style JuMP smoke test, quiet mode, common solver attributes, and randomness-robust result/objective/value assertions.
julia --project=. -e 'import Pkg; Pkg.test()'passed locally: package metadata 7/7, README JuMP workflow 4/4, QUBODrivers suite 130/130.gh pr checks 22shows all four CI jobs passing.
Merge-readiness: no blocking issues found; from a technical review, this is merge-ready. I am the PR author, so this review is submitted as COMMENT; formal approval must come from another maintainer.
|
Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up items:
|
Summary:
PySA.Optimizer.MOI.Silent()plus reads, sweeps, replicas, and temperature bounds in the smoke test.JuMPas a direct test dependency so the README workflow can be exercised by the package tests.Tests:
julia --project=. -e 'import Pkg; Pkg.test()'passed.Closes #19