Document SpinGlassPEPS integration architecture#41
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues: none found in the changed documentation.
Nonblocking issues: none found.
Questions: none.
Tests run and outcomes:
julia --project=docs/ -e 'using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.instantiate()'passed.julia --project=docs/ docs/make.jl localpassed, including doctests and cross-reference checks.julia +1.12 --project=. -e 'using Pkg; Pkg.test()'passed.julia +1.10 --project=. -e 'using Pkg; Pkg.test()'did not reach test execution locally because the root manifest was resolved with Julia 1.12.6 and Pkg could not locateOpenSSL_jllunder Julia 1.10.11.- The PR CI LTS Ubuntu job is failing in the existing QUBODrivers fixed-variable suite when
TenSolver.Optimizeris exercised on a one-variable model;ITensorMPS.dmrgerrors that system size 1 is unsupported. The documentation job passed.
Merge as-is: not while the required test workflow is red. I did not find a PR-diff issue that needs an inline comment, but this should get a green test run or an explicit maintainer decision on the unrelated LTS failure before merge.
|
Reviewed the PR feedback. Commits pushed:
Main changes made:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
|
Pushed the requested CI fix. Commits pushed:
Main changes made:
Tests run:
Comments intentionally not addressed:
Remaining risks:
|
bernalde
left a comment
There was a problem hiding this comment.
GitHub rejected an APPROVE review from this authenticated account because it is the PR author, so this is submitted as a COMMENT review. The maintainer recommendation is still merge-ready as-is.
- Blocking issues: none.
- Nonblocking issues: none.
- Questions: none.
- Tests run and outcomes:
git diff --check origin/main...HEAD: passed.julia +1.12 --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/qubo.jl")': passed, 82 tests.julia --project=docs/ docs/make.jl local: passed.julia +1.12 --project=. -e 'using Pkg; Pkg.test()': passed.gh pr checks 41 --watch --fail-fast: all PR checks passed across Julia 1, LTS, prerelease, docs build, and Documenter deploy.
- Merge recommendation: merge-ready as-is.
bernalde
left a comment
There was a problem hiding this comment.
GitHub rejected an APPROVE review from this authenticated account because it is the PR author, so this is submitted as a COMMENT review. The maintainer recommendation is still merge-ready as-is.
Blocking issues: none.
Nonblocking issues: none.
Questions: none.
Tests run and outcomes:
git diff --check origin/main...HEAD: passed.julia --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/qubo.jl")': passed, 82 tests.julia --project=. -e 'using Pkg; Pkg.test()': passed on Julia 1.11.5.julia --project=docs/ -e 'using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.instantiate()': passed.julia --project=docs/ docs/make.jl local: passed.julia +1.10 --startup-file=no -e 'using Pkg; Pkg.activate(; temp=true); Pkg.develop(PackageSpec(path=pwd())); Pkg.add(["Aqua", "JuMP", "QUBODrivers"]); using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/qubo.jl"); include("test/external.jl")': passed; QUBO 82/82, QUBODrivers 129/129, Aqua 11/11.- One-variable smoke checks for integer QUBO input and single-variable DynamicPolynomials input: passed.
Merge as-is: yes. I found no actionable correctness, API, documentation, or maintainability issues in this PR.
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; one process item below for the reviewer to decide.
Diff verification: merge-base ca27880; git diff <merge-base>..HEAD --stat = 6 files, +301 / -1, matching the PR's reported totals. Base = main; this is 1/6 of the SpinGlassPEPS stack (issues 35-40).
Issue intent (#35): #35 asks for a documentation-only PR adding the integration design doc, with explicit acceptance criteria "No new runtime dependencies" and "No public TenSolver solver behavior changes," and "Adding backend dispatch" listed out of scope. The design doc itself fully delivers the requested content (solver boundary, optional PEPS backend, DMRG-stays-default, packaging/optionality, staged plan) and is technically accurate against #35 and arXiv:2411.16431. Project.toml/docs/Project.toml deps are unchanged; SpinGlassPEPS is not added.
Nonblocking
- Scope: the PR is labeled documentation-only but bundles a real solver behavior change —
src/solver.jl:187adds_single_variable_minimizeplus dispatch in_minimize, sominimize/maximizeon a 1x1 QUBO now take an exact path instead of calling DMRG (which can't do system size 1). This contradicts #35's "documentation-only / no public behavior change / no backend dispatch" boundary. The PR body discloses it, and the change is correct and test-covered (energies -2.0, -1.0, 2.0 verified; multi-var DMRG path unaffected), so I am not blocking — but the same single-variable fix is also appearing in #32/#33/#34. Recommend peeling it into its own bug-fix PR so the stack base stays doc-only and the fix is reviewed once, on its own merits; or relabel #41 to drop the "documentation-only" framing.
Doc accuracy and wiring (verified, all correct): claims about optional structured-graph backend, DMRG default, SpinGlassPEPS reexports, PEPS+boundary-MPS+B&B, Julia 1.11/GPU footprint, and the 6-PR plan all match #35. The new page is registered in docs/make.jl, docs/src/index.md, and docs/DOCUMENTATION.md; all three @ref links (minimize, maximize, Solution) resolve; the only external link is the valid arXiv URL.
Question
2. Closes #35 is appropriate here — #35 is the design-doc issue, not the umbrella epic, so closing it on merge does not prematurely close issues 36-40. Confirmed; no action needed.
Tests/build run (Julia 1.10.11):
julia --project=docs -e 'Pkg.develop(path=pwd()); Pkg.instantiate()'— passed.julia --project=docs docs/make.jl— passed: Doctest, ExpandTemplates, CrossReferences, CheckDocument, Render all completed; only the benign "skipping deployment" notice (expected locally). The new page rendered.- Targeted solver smoke test (multi-var DMRG path + the three single-variable paths) — all correct, matching
test/qubo.jlexpectations. I did not run fullPkg.test()here; the targeted run confirms the solver change is non-regressing.
Merge readiness: The docs portion is ready — accurate, complete against #35, properly wired, builds cleanly, links resolve. The one item for the reviewer is the scope question: the PR ships a solver change under a documentation-only label. The code is correct and tested, so it is not a technical blocker, but consider splitting it out to keep the stack base clean. The formal verdict must come from another maintainer.
| return _minimize(H, cte, a -> p(vs => a); cutoff, kwargs...) | ||
| end | ||
|
|
||
| function _single_variable_minimize(::Type{T}, sites, obj, initial_time; device, verbosity, on_iteration, callback_every) where {T} |
There was a problem hiding this comment.
Nonblocking (scope): this exact single-variable path is a real solver behavior change in a PR labeled documentation-only, contradicting issue #35's "no public behavior change / no backend dispatch" boundary. It's correct and tested, so not a blocker — but the same fix also appears in #32/#33/#34. Consider peeling it into its own bug-fix PR so the stack base stays doc-only, or relabel this PR.
bernalde
left a comment
There was a problem hiding this comment.
Senior maintainer review of PR #41 ("Document SpinGlassPEPS integration architecture"). Reviewed on its own merits only, not the downstream stack. Author is my own posting identity, so GitHub forbids APPROVE/REQUEST_CHANGES from me; this is submitted as COMMENT. A formal approving/blocking verdict must come from another maintainer.
The documentation itself is accurate and well framed. The new page repeatedly uses "planned", "should", and an explicit disclaimer that it "is a design note for the implementation stack, not a description of behavior available in the current TenSolver release." Claims I spot-checked against the code hold: Julia 1.10 support (julia = "1.10" in Project.toml), MPO+DMRG over Boolean qudit sites, Solution wrapping the MPS plus convergence traces, PUBO via MultivariatePolynomials, and the QUBOTools SampleSet optimizer result. The @ref targets minimize, maximize, and Solution are all documented in docs/src/api.md, so cross-references resolve. The documenter/deploy check passed against this exact head SHA (PR41 preview built). Navigation and @contents updates are consistent.
The problems are about scope and base freshness, not the prose.
Blocking
-
Out-of-scope code change duplicates work already on main. This PR is supposed to be documentation-only (issue #35: "This PR should be documentation-only"; Out of scope explicitly lists "Adding backend dispatch"). But it also modifies
src/solver.jl(+35) andtest/qubo.jl(+36) to add the one-variable QUBO fix (_single_variable_minimize). That same fix is already onorigin/mainvia PR #48 (commit 61c8def, "fix: handle one-variable QUBOs"). The added code and tests are byte-for-byte identical to what main already contains. The reason it is here is that this branch was cut from old main (merge-base ca27880) before #48 landed. Net effect on a docs-only PR: it carries a redundant code change that does not belong to issue #35, muddying the "documentation-only" contract and the diff. Fix: rebase this branch onto currentorigin/main(9a3dd5c) and drop thesrc/solver.jlandtest/qubo.jlchanges so the PR contains only the four docs files. After rebasing they should disappear from the diff since they are identical to main. -
Closes #35on a documentation-only PR is appropriate here, with one caveat. Issue #35 explicitly scopes itself to the design document only (acceptance criteria: "A design document is added ...", "No new runtime dependencies", "No public TenSolver solver behavior changes"; out of scope: implementing conversions/dispatch). SoCloses #35is the correct linkage for this docs page and is not a mis-closure of an implementation request. This is downgraded from a closure-correctness block, but it interacts with item 1: the PR currently DOES change public solver behavior (the one-variable path), which violates issue #35's own acceptance criterion "No public TenSolver solver behavior changes." Removing the code change per item 1 resolves both. No action needed on theCloseskeyword itself once the diff is docs-only.
Nonblocking
-
Stale base. The branch is 5 commits behind
origin/main(cut from ca27880; main is at 9a3dd5c, v0.2.0; this head still hasversion = "0.1.0"inProject.toml). Rebasing onto current main is needed anyway for item 1 and matches the branch-hygiene note on issue #35 ("Keep the PR ... based on the repository default branch (main) after fetching and fast-forwarding"). -
PR description "Tests run" section lists running the full test suite and the one-variable QUBO assertions. For a docs-only PR (post-rebase) those become irrelevant; trim the description so it reflects the actual docs change and records the Documenter build as the relevant check.
Questions
- The page asserts "SpinGlassPEPS 1.5.0 declares Julia 1.11 compatibility" and unit-cell sizes "up to 24 spins for Pegasus and 16 spins for Zephyr." These are external-version-specific facts that will drift. Are they pinned to a SpinGlassPEPS release you have verified, and is it worth softening to "at the time of writing" so the doc does not silently go stale?
Doc build / tests
I could not run the Documenter build locally: the review sandbox denied git clone, repo tarball download, and writing the cloned tree under /tmp (julia 1.10.11 is present, but there was no checked-out tree to build, and +1.12 is not installed here). Falling back to CI: the documenter/deploy check is green against head 753ff05 and published the PR41 preview, so the doc build is healthy. All 11 CI jobs (Julia 1/lts/pre on ubuntu/macOS/windows, build, documenter) pass.
Summary
- Blocking: (a) docs-only PR carries a redundant
src/solver.jl+test/qubo.jlone-variable fix already present on main via #48 - rebase onto current main and drop it; this also restores compliance with issue #35's "No public solver behavior changes" criterion. - Nonblocking: stale base (5 behind, version still 0.1.0) - rebase; PR description test list no longer matches a docs-only change.
- Questions: external version/topology facts (SpinGlassPEPS 1.11 compat, Pegasus/Zephyr spin counts) may go stale - consider "at time of writing".
- Tests/doc-build: local Documenter build not run (sandbox denied clone/tarball/tree write); relied on CI -
documenter/deploygreen on head 753ff05, all checks pass. - Merge-readiness: the documentation content is accurate and ready, but the diff is not docs-only as intended. I would not merge this until the blocking issues above are addressed. Formal verdict to be recorded by another maintainer.
|
Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up items:
|
Summary
origin/mainso the PR no longer carries the out-of-scope one-variable solver change.Tests run
git diff --check origin/main...HEAD- passed.julia +1.12 --project=docs/ -e 'using Pkg; Pkg.instantiate()'- passed.julia +1.12 --project=docs/ docs/make.jl- passed; deployment skipped outside CI.julia +1.12 --project=. -e 'using Pkg; Pkg.test(; coverage=false)'- passed.Notes
main, not by this PR.Closes #35