Add optional SpinGlassPEPS backend#44
Conversation
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
GitHub rejected REQUEST_CHANGES because the authenticated account owns this pull request. The findings below are blocking from a maintainer review perspective. I would not merge this until the blocking issues above are addressed.
| _normalize_backend(backend::DMRGBackend) = backend | ||
| _normalize_backend(backend::PEPSBackend) = backend | ||
| _normalize_backend(backend::AbstractTenSolverBackend) = backend | ||
| function _normalize_backend(backend::Symbol) |
There was a problem hiding this comment.
Blocking: This reintroduces the hard-coded Symbol backend normalization that the current base just fixed. Because every symbol reaches this method, a package extension cannot add TenSolver._normalize_backend(::Val{:peps}) or another symbol-specific mapping; backend = :peps will always throw before extension-specific _minimize or _solve_ising methods can run. It also drops the fake-symbol regression test from test/backend.jl relative to the current base.
Please keep the current-base Val dispatch structure and layer PEPSBackend on top of it: _normalize_backend(backend::Symbol) = _normalize_backend(Val(backend)), _normalize_backend(::Val{:dmrg}) = default_backend, and a Val fallback that throws _backend_error(backend). Then keep/add a test proving a symbol-specific backend can be registered by an extension.
There was a problem hiding this comment.
Addressed by the restacked base and preserved here. The branch now inherits 009bcd1, which restores _normalize_backend(backend::Symbol) = _normalize_backend(Val(backend)), the Val{:dmrg} method, the Val fallback, and the fake-symbol regression test in test/backend.jl. No additional change was needed in e122868556e03840b5124333c6410b974e35a3db.
| SpinGlass component imports and calls. This keeps ordinary TenSolver installs | ||
| on the existing dependency footprint. | ||
|
|
||
| The extension is intentionally experimental while the upstream dependency stack |
There was a problem hiding this comment.
Blocking: This documents that the registered SpinGlass component compat bounds do not resolve with TenSolver, and the tests skip the only extension exercise when those packages are absent. That means the PR exports and documents PEPSBackend as current public behavior, but users cannot activate it from registered packages and CI never runs the implementation in ext/TenSolverSpinGlassPEPSExt.jl. I confirmed the resolution failure in a fresh environment by developing this PR and adding SpinGlassNetworks, SpinGlassEngine, and SpinGlassTensors; Pkg reports no compatible Parsers version because SpinGlassNetworks 1.4 constrains CSV/Parsers while the TenSolver environment constrains InlineStrings/Parsers.
Please either keep this as non-public scaffolding until the optional stack resolves, or update the dependency/compat strategy so the optional packages install together with TenSolver and add a CI/test path that exercises the extension.
There was a problem hiding this comment.
Addressed in e122868. PEPS backend/topology/result types are no longer exported or listed in public API docs, and docs/src/spinglasspeps_integration.md now describes the bridge as non-public scaffolding until the SpinGlass dependencies resolve and CI can cover the extension.
|
|
||
| sample(psi::Solution, n :: Integer) = [sample(psi) for _ in 1:n] | ||
|
|
||
| function sample(psi::PEPSSolution) |
There was a problem hiding this comment.
Nonblocking: sample(::PEPSSolution) returns the most probable retained state deterministically rather than sampling according to probabilities, so sample(psi, n) repeats the same state. That is surprising next to sample(::Solution), which samples from the represented distribution, and next to the public probabilities field. Consider sampling from the cumulative probability distribution, or exposing deterministic best-state selection through a separate helper if that behavior is intentional.
There was a problem hiding this comment.
Addressed in e122868. sample(::PEPSSolution) now draws from cumulative probability weights and validates length, nonnegative weights, and positive total weight. test/peps_backend.jl covers the weighted path and malformed probability vectors.
bernalde
force-pushed
the
feature/issue-37-backend-interface
branch
from
f4a3769 to
009bcd1
Compare
bernalde
force-pushed
the
feature/issue-38-spinglasspeps-backend
branch
from
cb9598f to
ca890fc
Compare
|
Pushed commit Main changes:
Tests run:
Notes on tests not cleanly runnable:
Comments intentionally not addressed:
Remaining risks/follow-up:
|
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Fresh review of the current net diff: I did not find blocking issues. The PEPS scaffold is now kept out of exports/public API docs, and the restored Val backend dispatch preserves the extension hook. I left two nonblocking comments on test coverage and documentation wording.
| """ | ||
| function solve_ising end | ||
|
|
||
| function solve_ising(model::IsingModel; backend=default_backend, kwargs...) |
There was a problem hiding this comment.
Nonblocking: solve_ising is now exported and listed in the API reference, but the PR only tests a one-spin field-only call through the IsingModel method. That does not exercise pair couplings, offset preservation, or the public solve_ising(J, h, offset) overload.
Please add a small exact 2- or 3-spin regression test that brute-forces the Ising objective and asserts both solve_ising(model; ...) and solve_ising(J, h, offset; ...) return the optimum and a Boolean sample whose converted spin state has matching Ising energy. That would make the new public API safer to maintain while the PEPS path remains internal.
There was a problem hiding this comment.
Addressed in f3c4db1. test/backend.jl now brute-forces a two-spin Ising objective with pair coupling and offset, then checks both solve_ising(model; ...) and solve_ising(J, h, offset; ...) return the optimum and a sample whose spin energy matches it.
| behavior. | ||
|
|
||
| This stack step keeps the direct PEPS path as non-public scaffolding. The core | ||
| package contains internal backend, topology, `solve_ising`, and result |
There was a problem hiding this comment.
Nonblocking: This wording groups solve_ising into the non-public scaffolding, but solve_ising is exported and included in the public API reference. To keep the API boundary clear, please reword this to say the PEPS backend/topology/result types are internal scaffolding, while solve_ising is the public Ising boundary that optional structured backends may implement.
There was a problem hiding this comment.
Addressed in f3c4db1. The integration note now says backend/topology/result boundaries are internal scaffolding, while exported solve_ising is the public Ising boundary optional structured backends may implement.
|
Pushed commit Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks/follow-up:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/issue-37-backend-interface #44 +/- ##
=====================================================================
Coverage ? 72.84%
=====================================================================
Files ? 6
Lines ? 475
Branches ? 0
=====================================================================
Hits ? 346
Misses ? 129
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Reviewed the current net diff against feature/issue-37-backend-interface. No blocking or nonblocking inline issues found. The PEPS scaffolding remains internal, solve_ising is covered as public API, and the updated tests cover the newly public behavior and internal boundary sufficiently for this PR.
GitHub would not let me submit APPROVE because this account is the PR author: "Review Can not approve your own pull request".
Tests run locally:
- git diff --check origin/feature/issue-37-backend-interface...HEAD
- julia --project=. -e "using Pkg; Pkg.test()"
- julia --project=. -e "using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/backend.jl"); include("test/peps_backend.jl")"
- julia +1.12 --project=. -e "using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/backend.jl"); include("test/peps_backend.jl")"
- julia --project=. targeted solve_ising exact-energy probe
- julia --project=docs/ -e "using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.instantiate()"
- julia --project=docs/ docs/make.jl local
The optional SpinGlassPEPS extension path still cannot be exercised locally because SpinGlassNetworks/SpinGlassEngine/SpinGlassTensors cannot currently resolve with this package dependency stack; the PR documents that limitation and keeps the PEPS backend internal.
Merge recommendation: merge-ready from this review pass.
1 participant
Summary
SquareGrid,KingGrid,PEPSBackend,solve_ising, andPEPSSolution.TenSolverSpinGlassPEPSExtpackage extension that translates structured Ising/QUBO inputs into SpinGlassNetworks/SpinGlassEngine calls and decodes PEPS states back to TenSolver Boolean states.Tests run
julia +1.12 --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/peps_backend.jl")'git diff --checkjulia --project=docs/ docs/make.jl localjulia +1.12 --project=. -e 'using Pkg; Pkg.test()'julia +1.12 --startup-file=no -e 'Meta.parse(read("ext/TenSolverSpinGlassPEPSExt.jl", String)); println("extension syntax parsed")'julia +1.12 -e 'using Pkg; env = "/tmp/tensolver-qubotools011-check"; Pkg.activate(env); Pkg.develop(path="/home/bernalde/repos/TenSolver.jl"); Pkg.add(Pkg.PackageSpec(name="QUBOTools", version="0.11")); Pkg.instantiate(); Pkg.status(); using TenSolver; println("loaded with QUBOTools 0.11")'Notes about tests not run
SpinGlassTensors,SpinGlassNetworks,SpinGlassEngine, and this branch failed to resolve against current registered compat bounds. After broadening TenSolver's QUBOTools compat to include 0.11, the remaining resolver conflict isSpinGlassNetworks1.4 requiringCSV = "0.8"/old Parsers while TenSolver's ITensors/NDTensors stack requires InlineStrings with newer Parsers compatibility.Closes #38