Add JuMP PEPS backend attributes#45
Conversation
cb9598f to
ca890fc
Compare
078e097 to
873f02b
Compare
873f02b to
ab04073
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/issue-38-spinglasspeps-backend #45 +/- ##
==========================================================================
+ Coverage 72.84% 73.44% +0.60%
==========================================================================
Files 6 6
Lines 475 546 +71
==========================================================================
+ Hits 346 401 +55
- Misses 129 145 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bernalde
left a comment
There was a problem hiding this comment.
GitHub rejected REQUEST_CHANGES because the authenticated account owns this pull request. The blocking comment below is blocking from a maintainer review perspective. I would not merge this until the blocking issue above is addressed.
Blocking issue found: the JuMP-facing PEPS backend selection is public, but the optional SpinGlass component stack still does not resolve with TenSolver and the actual PEPS optimizer success path is skipped in CI. I would not merge this until the blocking issue above is addressed.
Tests run locally:
- git diff --check origin/feature/issue-38-spinglasspeps-backend...HEAD
- julia --project=. -e "using Pkg; Pkg.instantiate()"
- julia --project=test -e "using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.instantiate()"
- julia --project=test -e "using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/jump.jl")"
- julia --project=. -e "using Pkg; Pkg.test()"
- julia --project=docs/ -e "using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.instantiate()"
- julia --project=docs/ docs/make.jl local
- julia --project=/tmp/tensolver-pr45-opt -e "using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.add(["SpinGlassNetworks", "SpinGlassEngine", "SpinGlassTensors"])" failed with the known Parsers/CSV/InlineStrings resolver conflict
- local fake-PEPS JuMP integration probe passed, showing the missing success-path test can be covered without optional packages
| function _optimizer_backend(get) | ||
| backend = _optimizer_symbol(get("backend"), "backend") | ||
| backend === :dmrg && return :dmrg | ||
| backend === :peps && return _optimizer_peps_backend(get) |
There was a problem hiding this comment.
Blocking: This exposes backend = :peps through the JuMP optimizer, but the optional SpinGlass component stack still cannot be installed together with TenSolver and CI skips the only real PEPS optimizer solve when those packages are absent. I reproduced the resolver failure in a fresh environment: adding SpinGlassNetworks, SpinGlassEngine, and SpinGlassTensors conflicts through SpinGlassNetworks -> CSV/Parsers and TenSolver/QUBOTools -> InlineStrings/Parsers. That leaves this new public attribute path without a runnable registered install path or CI coverage of the success branch. Please either keep the PEPS JuMP attributes and public docs internal until the optional stack resolves, or fix the dependency/compat strategy and add CI coverage. At minimum, add a fake-extension integration test that defines TenSolver._solve_ising(::PEPSBackend, ...) and drives JuMP.optimize! through this branch so the SampleSet and metadata path is covered without the optional packages.
There was a problem hiding this comment.
Addressed in f674a37: added a fake AbstractStructuredTopology JuMP test that drives backend = :peps through JuMP.optimize!, then checks objective/primal values plus QUBOTools solution metadata and reads in test/jump.jl. The optional real SpinGlass install path remains a follow-up once that dependency stack resolves.
|
|
||
| samples = Vector{QUBOTools.Sample{T,Int}}(undef, num_reads) | ||
| for i in 1:num_reads | ||
| x = states[mod1(i, length(states))] |
There was a problem hiding this comment.
Nonblocking: This cycles retained PEPS states evenly and ignores psi.probabilities, while sample(::PEPSSolution) samples by those weights and num_reads is exposed as a sampling count. The resulting SampleSet read counts can misrepresent the PEPS distribution for downstream QUBOTools consumers. Consider generating samples with sample(psi) for num_reads, or converting probabilities into reads counts while preserving the full retained-state data under metadata[\"peps\"].
There was a problem hiding this comment.
Addressed in f674a37: _qubo_samples(::PEPSSolution, ...) now maps psi.probabilities to deterministic QUBOTools reads counts instead of cycling states. test/jump.jl covers the weighted read counts.
There was a problem hiding this comment.
Still addressed after the restack. The probability-weighted PEPS reads adaptation from f674a37 remains in place, and the updated tests pass on the current branch head.
|
Pushed commit:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks/follow-up:
|
bernalde
left a comment
There was a problem hiding this comment.
Final focused review of f674a37: no blocking or nonblocking issues found.
GitHub would not let this account submit APPROVE because it owns the pull request: "Review Can not approve your own pull request".
The new fake PEPS JuMP integration test now exercises backend = :peps through JuMP.optimize!, validates objective/primal values, and checks QUBOTools metadata/read counts without requiring the unresolved optional SpinGlass package stack. The PEPS SampleSet adaptation now derives deterministic read counts from psi.probabilities instead of cycling states evenly.
Evidence inspected:
- Current PR head is f674a37.
- GitHub checks are green for Julia lts/1/pre across Ubuntu, Windows, and macOS.
- Documentation build and documenter/deploy are green.
- Local focused tests already run on this head:
git diff --check f72bb06..f674a37, focusedtest/jump.jlon Julia 1.11.5 and Julia 1.12.6, and a targeted_peps_read_countsprobe; all passed, with the expected broken optional SpinGlass solve.
Merge recommendation: merge-ready from this focused review pass. The real SpinGlassPEPS dependency resolver issue remains an external follow-up before relying on the real optional backend in CI.
bernalde
left a comment
There was a problem hiding this comment.
Senior maintainer review. Note: I am the PR author, so GitHub only permits a COMMENT event from me; a formal approval or change-request must come from another maintainer. No blocking issues found.
Diff verification: confirmed against the stacked base feature/issue-38-spinglasspeps-backend (not main). merge-base f3c4db1; git diff <merge-base>..HEAD --stat = 5 files, +479 / -23, matching the PR's reported totals.
Issue intent (#39): expose the PEPS backend through QUBODrivers/JuMP raw attributes (backend + peps_*), keep DMRG default, require explicit topology, fail clearly without the extension, add SampleSet metadata, add guarded tests. All acceptance criteria are addressed; the change is additive and default-preserving.
Verified: all 12 attributes (backend + 11 peps_*) are both declared and read — no declared-but-unread or read-but-undeclared bugs. backend/peps_layout/peps_strategy are typed Union{Symbol,String} and normalized (lowercased/stripped). Missing-topology raises ArgumentError mentioning peps_topology. backend=:peps without the extension fails with a clear, actionable error (tested). Default behavior unchanged: the DMRG kwarg block is the original call plus backend, and :dmrg re-normalizes to default_backend. The _peps_read_counts largest-remainder apportionment is correct and guards empty/negative/mismatched inputs.
Nonblocking
- The public attribute
peps_truncationis plumbed intoPEPSBackend(local_dimension=...)— i.e. it sets the per-site local Hilbert-space dimension, not an MPS bond truncation as the name suggests (src/TenSolver.jl:62,:129). Rename topeps_local_dimension, or document explicitly thatpeps_truncationmaps to local dimension. - The optimizer default
peps_strategy = :svddiffers from thePEPSBackendconstructor defaultcontraction = :auto; since the optimizer always forwards the attribute, JuMP users can't reach:autowithout setting it explicitly. Deliberate and consistent with #39, but worth a doc note.
Question
3. "preprocess" :: Bool = false is declared but never read — I confirmed via git show <merge-base> that this is pre-existing (present at the stacked base, unchanged here), so it is out of scope for this PR. Flagging so it isn't mistaken for a new dead attribute; worth cleaning up separately.
Tests run (Julia 1.10.11):
julia --project=. -e 'using Pkg; Pkg.instantiate(); Pkg.test()'— passed (Testing TenSolver tests passed).JuMP backend attributes— 39 Pass, 1 Broken (the@test_skipfor the optional live-PEPS solve, correctly skipped because SpinGlassPEPS is absent on 1.10). Default / Explicit-DMRG / String-normalization / missing-topology-error / unavailable-extension-error / fake-PEPS plumbing+metadata all pass without the real extension. QUBODrivers.jl 130, Aqua.jl 11.
Merge readiness: Ready to merge (pending normal human review, and #38 landing first). Additive, keeps DMRG as the verified default, plumbs all PEPS attributes correctly with matching names/types/defaults, fails clearly when backend=:peps is selected without the extension, and ships well-guarded tests. Only nonblocking polish: the peps_truncation -> local-dimension naming and the peps_strategy default note. Must merge into feature/issue-38-..., not main. The formal verdict must come from another maintainer.
| "peps_strategy" :: Union{Symbol, String} = :svd | ||
| "peps_num_sweeps" :: Int = 1 | ||
| "peps_transformations" :: Any = :all | ||
| "peps_truncation" :: Any = nothing |
There was a problem hiding this comment.
Nonblocking: peps_truncation is plumbed into PEPSBackend(local_dimension=...) (see the read at line 129), i.e. it sets the per-site local Hilbert-space dimension, not an MPS bond truncation as the name implies. Rename to peps_local_dimension, or document the mapping explicitly.
There was a problem hiding this comment.
Addressed in 1f90068. The JuMP attribute is now peps_local_dimension, the helper/error text uses that name, and the fake PEPS JuMP test sets the renamed attribute.
bernalde
left a comment
There was a problem hiding this comment.
Senior maintainer review of PR #45 "Add JuMP PEPS backend attributes". Reviewed the head (f674a37) diffed against the merge-base with its actual base branch feature/issue-38-spinglasspeps-backend (merge-base f3c4db1); totals reproduce the reported +479/-23 across 5 files, so the base is correct and findings below are scoped to changes introduced by #45, not inherited from the #41-#44 stack.
Note on process: I am the author of this PR, so GitHub will not let me APPROVE or REQUEST_CHANGES on it. This is posted as an event=COMMENT review; the formal verdict needs a different maintainer. There are no blocking correctness issues; the only hard blocker on merging is the stacked-base dependency and the stale-base conflict described below, both of which are environmental rather than defects in the diff.
What the PR does and whether it matches issue #39: it adds backend plus peps_* raw optimizer attributes, keeps DMRG as default, requires explicit topology metadata, constructs PEPSBackend, adapts PEPSSolution into QUBOTools samples, attaches namespaced PEPS metadata to the SampleSet, and documents the path. This maps cleanly onto issue #39's acceptance criteria (DMRG default, explicit DMRG/PEPS selection, required topology, exposed metadata, default + error-path tests). Pegasus/Zephyr layouts are not implemented, which #39 explicitly lists as optional ("only if direct API support already exists"), so that omission is acceptable. Closes #39 is therefore a correct link, with the caveat that #39's own comment notes this PR must not be marked ready until its prerequisite stack merges.
Attribute plumbing is correct. The new @setup entries cover the names #39 proposed; _optimizer_backend/_optimizer_peps_backend map peps_strategy->contraction and peps_truncation->local_dimension, and every keyword passed to PEPSBackend(...) matches the constructor signature in src/solver.jl. The DMRG path forwards backend = :dmrg (a Symbol) into minimize, which normalizes it via _normalize_backend(::Val{:dmrg}), so the default path is unchanged. The largest-remainder distribution in _peps_read_counts is correct (verified by hand: probs [0.75,0.25], num_reads 3 -> [2,1]; matches the unit test), and the empty/zero/negative/total-weight edge cases all throw or short-circuit sensibly.
Tests: I attempted to run Pkg.test() once in a worktree at the head SHA with the sandbox override; execution was denied by the environment (consistent with sibling reviews), so I could not run the suite locally. I fell back to static review plus CI. All 11 check-runs (Julia 1/lts/pre x ubuntu/macOS/windows, build, docs) are green against the exact head SHA f674a37. Important caveat: that CI ran on the stale base chain cut from old main (ca27880), not against current origin/main (v0.2.0, 9a3dd5c), so green CI does not prove the code builds on today's main.
Merge-readiness / blocking environmental issues:
- Stacked base. The base branch is
feature/issue-38-spinglasspeps-backend(head of PR #44), notmain. This PR cannot merge until the prerequisite chain #41-#44 merges and the base is retargeted tomain. - Stale base + attribute-syntax conflict. Current
mainmigrated the@setupattributes to the QUBODrivers 0.6 typed/named syntax (e.g.Cutoff["cutoff"] :: Float64 = 1e-8) and addedhonors_final_reads/enforces_time_limitplus aFinalNumberOfReads-basedsamplebody. This PR adds its 12 new attributes in the OLD"string" :: Type = defaultform and rewrites the oldsamplebody. Rebasing onto current main will conflict insrc/TenSolver.jlboth in the@setupblock and inQUBODrivers.sample; the new attributes will need to be re-expressed in the 0.6 named form and the backend dispatch re-applied on top of main's refactoredsample. This must be reconciled before merge.
Nonblocking:
- Divergent defaults between the two entry points. The JuMP
@setupdefaults (peps_beta = 1.0,peps_strategy/contraction = :svd) differ from the directPEPSBackendconstructor defaults (beta = 2.0,contraction = :auto). The JuMP values follow #39's proposed list, so this is defensible, but the divergence is a future foot-gun; consider a one-line comment near the@setupblock noting the JuMP defaults intentionally differ from the constructor. - Asymmetric
backendmetadata. PEPS runs setmetadata["backend"], but the DMRG fallback_add_backend_metadata!(metadata, psi) = metadataleaves no"backend"key. Harmless and within #39's spec, but settingmetadata["backend"] = "dmrg"(or similar) on the DMRG path would make SampleSet metadata uniform across backends.
Questions:
graduate_truncationandno_cachePEPSBackendkeywords are not exposed through the optimizer. They are outside #39's proposed attribute list, so presumably intentional, but confirm whether they should be reachable from JuMP for parity with the direct API.peps_transformationsis typedAnyand forwarded verbatim. Is there any intended validation of accepted values (e.g.:all,:identity, a vector), or is validation deliberately deferred to the extension?
Summary:
- Blocking (code): none.
- Nonblocking: divergent JuMP-vs-constructor defaults; asymmetric
backendmetadata on the DMRG path. - Questions: unexposed
graduate_truncation/no_cache; intended validation ofpeps_transformations. - Tests run: local
Pkg.test()was denied by the sandbox; fell back to static analysis + CI. All 11 check-runs pass against headf674a37, but CI ran on the stale base chain (old main), not currentorigin/mainv0.2.0. - Merge-readiness: not mergeable yet. It is stacked on
feature/issue-38-spinglasspeps-backendand must wait for #41-#44 to merge, then be retargeted tomainand reconciled with main's QUBODrivers 0.6 attribute migration (guaranteed conflict insrc/TenSolver.jl). The diff itself is correct and well-tested.
Given the stacked-base dependency and the stale-base conflict, I would not merge this until the blocking issues above are addressed.
| # PEPS backend keywords | ||
| "peps_topology" :: Any = nothing | ||
| "peps_layout" :: Union{Symbol, String} = :square | ||
| "peps_beta" :: Float64 = 1.0 |
There was a problem hiding this comment.
Nonblocking: this JuMP default peps_beta = 1.0 differs from the PEPSBackend constructor default beta = 2.0 in src/solver.jl. It follows #39's proposed list, so it's defensible, but the two entry points now disagree. A short comment noting the divergence would prevent confusion.
There was a problem hiding this comment.
Addressed in 1f90068. The JuMP peps_beta default now matches PEPSBackend(beta = 2.0).
| "peps_max_states" :: Int = 256 | ||
| "peps_cutoff_prob" :: Float64 = 0.0 | ||
| "peps_onGPU" :: Bool = false | ||
| "peps_strategy" :: Union{Symbol, String} = :svd |
There was a problem hiding this comment.
Nonblocking: peps_strategy defaults to :svd here, but PEPSBackend's contraction defaults to :auto. Same intentional-but-divergent-defaults concern as peps_beta. Confirm :svd is the intended JuMP default.
There was a problem hiding this comment.
Addressed in 1f90068. The JuMP peps_strategy default now matches PEPSBackend(contraction = :auto).
| return metadata | ||
| end | ||
|
|
||
| _add_backend_metadata!(metadata::Dict{String,Any}, psi) = metadata |
There was a problem hiding this comment.
Nonblocking: the DMRG fallback leaves no metadata["backend"] key, while the PEPS method sets one. Within #39's spec, but setting metadata["backend"] = "dmrg" here would make SampleSet metadata uniform across both backends.
There was a problem hiding this comment.
Addressed by the current restack rather than by adding a raw string key. Both backends now use QUBODrivers metadata with metadata["backend"] as the backend record; PEPS-specific details are under metadata["tensolver"]["peps"]. The relevant tests are updated in 1f90068.
…ackend' into feature/issue-39-jump-peps-attrs # Conflicts: # src/TenSolver.jl
|
Pushed commits:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks/follow-up:
|
Summary
:dmrg/"dmrg"selection.PEPSBackendconstruction, QUBOTools SampleSet adaptation, and PEPS metadata undermetadata["tensolver"]["peps"].PEPSBackendforpeps_betaandpeps_strategy;peps_local_dimensionmaps toPEPSBackend(local_dimension=...).Tests run
julia +1.12 --project=. -e 'using Pkg; Pkg.instantiate()'- passed.julia +1.12 --project=test -e 'push!(LOAD_PATH, pwd()); using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/jump.jl")'- passed.julia +1.12 --project=test -e 'push!(LOAD_PATH, pwd()); using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/peps_backend.jl")'- passed.julia +1.12 --project=test -e 'push!(LOAD_PATH, pwd()); using Test, Random, LinearAlgebra, TenSolver; include("test/external.jl")'- passed.julia +1.12 --project=docs/ -e 'using Pkg; Pkg.instantiate()'- passed.julia +1.12 --project=docs/ docs/make.jl- passed.julia +1.12 --project=. -e 'using Pkg; Pkg.test(; coverage=false)'- passed.git diff --check- passed.Notes
Closes #39