Add optional SpinGlassPEPS backend

[bernalde]

Summary

• Adds an optional SpinGlassPEPS package extension behind PEPSBackend while keeping DMRG as the default backend.
• Uses the native minimize API for spin-domain inputs (domain = [-1, 1]); the separate solve_ising entry point and the obsolete IsingModel adapter have been removed.
• Introduces an abstract Solution interface with backend-specific DMRGSolution and PEPSSolution implementations sharing sample, prob, and is_feasible behavior.
• Keeps PEPS configuration and transformation helpers inside the extension, validates PEPSSolution invariants at construction, stores raw backend results in metadata, and propagates transformation failures.
• Accepts only quadratic polynomial inputs and native spin domains for the PEPS path, avoiding the removed QUBO-to-Ising round trip.
• Merges current main and updates the implementation, documentation, and tests to the current TenSolver backend and domain APIs.

Tests run

• git diff --check origin/main...HEAD — passed.
• JuliaFormatter validation for ext/TenSolverSpinGlassPEPSExt.jl and src/backends/peps.jl — passed.
• Backend interface tests — 24 passed.
• Ising conversion tests — 3,611 passed.
• PEPS core tests — 39 passed; the optional SpinGlassPEPS extension test remains skipped when the optional component stack is unavailable.
• julia +release --project=. --startup-file=no -e 'using Pkg; Pkg.test(; coverage=false)' — passed.
• julia +release --project=docs/ --startup-file=no docs/make.jl — passed after dependency precompilation; deployment was skipped outside CI.

Notes

• The real SpinGlassPEPS CPU solve path still cannot be exercised in the default environment because the optional SpinGlass component stack is unavailable there. The extension remains experimental and reports a clear loading error without those packages.
• This PR references [SpinGlassPEPS 4/6] Add optional SpinGlassPEPS-backed structured solver path #38 rather than closing it. The issue should remain open until a small structured-grid CPU solve runs through the real optional stack.

Refs #38

[bernalde]

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.

[bernalde]

Pushed commit e122868556e03840b5124333c6410b974e35a3db.

Main changes:

• Kept the PEPS bridge as non-public scaffolding while the registered SpinGlass component stack does not resolve with TenSolver and CI cannot exercise the extension.
• Removed PEPS backend/topology/result exports and public API docs, while keeping internal qualified access for extension development.
• Updated the integration note to describe the PEPS path as gated internal scaffolding rather than current public behavior.
• Changed sample(::PEPSSolution) to sample by cumulative probability weights and added regression coverage for weighted sampling plus malformed probability vectors.
• The outdated Symbol-dispatch thread is already addressed by the current Introduce solver backend interface #43 base: PR Add optional SpinGlassPEPS backend #44 now has the Val dispatch hook and fake-symbol backend test.

Tests run:

• julia +1.12 --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/backend.jl"); include("test/peps_backend.jl")' - passed; backend interface 19 pass, PEPS backend core 34 pass, optional SpinGlassPEPS extension 1 expected broken.
• julia +1.12 --project=. -e 'using Pkg; Pkg.test()' - passed.
• julia +1.11.5 --project=docs/ docs/make.jl local - passed.
• git diff --check - passed.

Notes on tests not cleanly runnable:

• julia +1.12 --project=docs/ docs/make.jl local reaches doctests and document checks but fails during HTML rendering with could not find source path for package MbedTLS_jll from docs/Manifest.toml. The docs manifest was resolved with Julia 1.11.5, and the same docs command passes under +1.11.5.

Comments intentionally not addressed:

• The outdated Symbol-dispatch thread did not need new code in this commit because the restacked Introduce solver backend interface #43 base already contains the fix.

Remaining risks/follow-up:

• Downstream stacked PRs Add JuMP PEPS backend attributes #45 and Add PEPS docs, examples, and benchmarks #46 will need to be rebased after this branch update.
• The SpinGlass component stack still needs a resolvable compat/CI path before the PEPS backend should become public.

[bernalde]

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.

[bernalde]

Pushed commit f3c4db10db4af978bb04f1ef9c804604f8b92c1a.

Main changes:

• Added an exact two-spin solve_ising regression covering pair couplings, offset preservation, the IsingModel method, and the public solve_ising(J, h, offset) overload.
• Clarified the SpinGlassPEPS integration note so PEPS backend/topology/result boundaries are internal scaffolding while exported solve_ising remains the public Ising boundary optional structured backends may implement.

Tests run:

• julia +1.12 --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/backend.jl")' - passed; backend interface 25 pass.
• julia +1.12 --project=. -e 'using Pkg; Pkg.test()' - passed.
• julia +1.11.5 --project=docs/ docs/make.jl local - passed.
• git diff --check - passed.

Comments intentionally not addressed:

• None in the fresh review. Previously replied outdated threads remain addressed by 009bcd1584589592e135d6e7524d34cd3c03e712 and e122868556e03840b5124333c6410b974e35a3db.

Remaining risks/follow-up:

• Downstream stacked PRs Add JuMP PEPS backend attributes #45 and Add PEPS docs, examples, and benchmarks #46 need to be rebased after this branch update.
• The SpinGlass component stack still needs a resolvable compat/CI path before the PEPS backend should become public.

[codecov]

Codecov Report

❌ Patch coverage is 33.14917% with 121 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (feature/issue-37-backend-interface@009bcd1). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
ext/TenSolverSpinGlassPEPSExt.jl	0.00%	112 Missing ⚠️
src/solver.jl	82.50%	7 Missing ⚠️
src/solution.jl	93.10%	2 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bernalde]

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.

[bernalde]

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 substantive Question for the reviewer below.

Diff verification: confirmed against the stacked base feature/issue-37-backend-interface (not main). merge-base 009bcd1; git diff <merge-base>..HEAD --stat = 10 files, +646 / -5, matching the PR's reported totals.

Issue intent (#38): add an optional SpinGlassPEPS-backed path without making SpinGlassPEPS a hard dependency; reuse the #42 conversions; decode back to Boolean and report the objective in TenSolver's convention; keep DMRG default; guard tests so absence of the heavy dep / Julia 1.10 does not break core CI. The PR satisfies all of these.

Optionality — PASS: the three SpinGlass packages are in [weakdeps] + [extensions] (TenSolverSpinGlassPEPSExt), not in [deps]. julia = "1.10" is left unchanged (no silent breaking bump); the extension mechanism is valid on 1.10.

Julia-compat reality (Question, not a blocker): I confirmed empirically on Julia 1.10.11 that SpinGlassNetworks@1.4/SpinGlassTensors@1.3 are unresolvable (resolver: "restricted by julia compatibility ... no versions left"). So on the only CI-able Julia (1.10) the weakdeps can never install, the extension never loads, and ordinary users are unaffected — but the entire SpinGlassPEPS bridge is therefore dead/untested-on-CI until TenSolver raises compat to 1.11. The PR is honest about this and keeps the PEPS types internal/unexported. For the reviewer to decide: merge now as the "experimental scaffolding" #38 explicitly permits (option 3), or defer activation until compat is raised to 1.11. Not blocking under #38's stated acceptance.

Bridge correctness (static; heavy dep not runnable here) — looks correct: _minimize(::PEPSBackend, ...) reuses qubo_to_ising; the reported objective is recomputed independently in TenSolver's convention via _decoded_records (decode Potts -> spins -> spin_to_bool -> ising_energy), not trusting SpinGlassPEPS's raw_energy (kept as metadata) — this sidesteps any sign/offset mismatch between the libraries. _ising_instance maps diagonal=biases, off-diag=couplings, consistent with the upper-triangular canonical J. The DMRG solve_ising round trip is verified by the new passing test.

Default unchanged — PASS. Tests guarded — PASS: test/peps_backend.jl gates the extension test on Base.find_package for all three components and uses @test_skip (observed as "Broken 1", i.e. skipped, on 1.10); the always-run "PEPS backend core" tests only the available types/validation.

Nonblocking

1. src/solution.jl narrows Base.in(bs, psi) to Base.in(bs::AbstractVector, psi). All in-repo callers pass vectors and tests pass, but this is a subtle public-API signature narrowing — confirm intended.
2. src/TenSolver.jl adds export Solution (previously unexported). Purely additive/non-breaking; PEPSSolution/PEPSBackend/SquareGrid/KingGrid are intentionally not exported.
3. ext/...Ext.jl _metadata reads SpinGlassEngine Solution fields (energies/probabilities/largest_discarded_probability); plausible but unverifiable without the dep — confirm against SpinGlassEngine 1.6 when the path is first exercised.

Tests run (Julia 1.10.11):

• julia --project=. -e 'using Pkg; Pkg.instantiate(); Pkg.test()' — passed (Testing TenSolver tests passed): Backend interface 25/25, PEPS backend core 34/34, Optional SpinGlassPEPS extension = 1 Broken (skipped as expected), QUBODrivers 128/128, Aqua 11/11, VRP/HDF5/Doctests pass.
• Adversarial install: Pkg.add(SpinGlassNetworks@1.4) on fresh 1.10 -> resolve failed with explicit julia-compat exclusion (confirms uninstallable on 1.10; expected, not a PR failure).

Merge readiness: No blocking issues. The PR honors #38: optionality via package extension (not [deps]), DMRG default preserved, conversions reused, objective decoded into TenSolver's Boolean convention, tests properly guarded, core suite green on 1.10. The substantive caveat is the Question above: the bridge is uninstallable/untested on CI Julia and remains so until compat is raised to 1.11; merge as documented scaffolding or defer, and track a follow-up to exercise it under 1.11. Stacked on #43 and must merge after it. The formal verdict must come from another maintainer.

[bernalde]

Question for the reviewer: these weakdeps require Julia 1.11, but TenSolver declares julia = "1.10". I confirmed SpinGlassNetworks@1.4 / SpinGlassTensors@1.3 are unresolvable on 1.10, so the extension can never load on the only CI-able Julia — the whole bridge is dead/untested on CI until compat is raised to 1.11. Optionality is correct (weakdeps, not deps), so not a blocker; decide whether to merge as experimental scaffolding (allowed by #38) or defer activation, and track a follow-up to exercise it under 1.11.

[bernalde]

Not changed in this pass. I am keeping this as experimental scaffolding rather than raising TenSolver's Julia compat or adding an activation path here. Follow-up remains to exercise the extension under Julia 1.11 or once the SpinGlass compat stack resolves.

[bernalde]

Addressed in 98a3573 by documenting that this PR is PEPS scaffolding, not final completion of #38. The PR body now uses Refs #38; I did not raise Julia compat or activate the SpinGlass stack here.

[bernalde]

Status update: the resolvability blocker is now precisely diagnosed and filed upstream. On Julia 1.10 (our LTS CI) the stack can never load because SpinGlassEngine 1.6 requires julia = "1.11". On Julia 1.11 the minimal failing pair is SpinGlassNetworks@1.4 + ITensors: NDTensors hard-depends on InlineStrings (Parsers 2.x) while SpinGlassNetworks pins CSV = "0.8" (Parsers ≤ 1.1.2). Filed as euro-hpc-pl/SpinGlassNetworks.jl#23 with a suggested compat widening. The scaffolding stance here is unchanged; activation is tracked on #38.

[bernalde]

The extension remains experimental scaffolding on the current head. I did not raise TenSolver's Julia compatibility or make the SpinGlass packages hard dependencies; the PR still uses Refs #38, and the real optional-stack CPU exercise remains follow-up work for #38.

[bernalde]

Status unchanged at current head 33764cb: the extension remains experimental scaffolding; TenSolver compatibility and weak-dependency status are unchanged, and the real optional-stack CPU exercise remains follow-up work under #38.

[bernalde]

Senior maintainer review of PR #44 ("Add optional SpinGlassPEPS backend"), diffed against the merge-base of origin/feature/issue-37-backend-interface and the PR head (009bcd1). Reproduced totals: 10 files, +646/-5. Only issues introduced by #44 itself are flagged; the #41-#43 base chain is treated as inherited.

Process note: I am the PR author, so GitHub only permits an event = COMMENT from this account. A formal approve/request-changes must come from another maintainer.

Test execution: the review sandbox denied julia and git (including with sandbox disabled), so I could not run the suite locally. Findings are from static review of the head sources plus CI. CI is green at the head SHA across all 12 check-runs (Julia 1/lts/pre x ubuntu/macOS/windows, build, docs, codecov). Caveat: CI ran with the stack cut from old main, and the optional SpinGlassPEPS path is @test_skip-ped because the component packages are not installed in CI, so the real PEPS solve and decode path is NOT exercised by any automated test.

Optionality mechanism (verified correct): the SpinGlass components are declared under [weakdeps] + [extensions] (TenSolverSpinGlassPEPSExt), not hard [deps]. Compat bounds are set (SpinGlassEngine 1.6, SpinGlassNetworks 1.4, SpinGlassTensors 1.3; QUBOTools widened to "0.10, 0.11"). When the extension is not loaded, PEPSBackend solves throw a clear ArgumentError naming the three packages and pointing to backend = :dmrg (covered by test/peps_backend.jl). This satisfies issue #38's "do not add SpinGlassPEPS as mandatory deps" requirement and the packaging-option-1 (extension) preference. Ordinary installs keep the existing dependency footprint.

Mapping correctness (static): _ising_instance builds the SpinGlassNetworks dict from model.h (diagonal) and the upper-triangle of model.J, which matches IsingModel's objective offset + sum(h_i s_i) + sum_{i<j} J_ij s_i s_j. The constant offset is intentionally dropped from the instance and reintroduced by recomputing the final energy via ising_energy(model, spins), so returned energies are in TenSolver's original convention. Decode uses decode_potts_hamiltonian_state then spin_to_bool. Square/king super_square_lattice clustering and the SquareSingleNode/KingSingleNode{GaugesEnergy} networks follow the issue's implementation sketch. Multi-spin-per-site is handled in _check_layout_edges by skipping intra-cell couplings (ci == cj). I did not find a correctness bug in the mapping by inspection, but see the no-coverage caveat below.

1. Blocking

None introduced by this PR. The mapping/decoding code is plausible but completely unexercised by CI (the only test that would run it is skipped). I am not raising this to Blocking because issue #38 explicitly anticipates this as packaging-option-3 ("clearly experimental branch-only prototype if dependency/version constraints block an extension") and the PR keeps the PEPS types unexported and out of the public API docs. But it is the single largest risk: 244 lines of extension logic ship with zero executed-path coverage. See Questions.

2. Nonblocking

Nonblocking: sample/prob/in on PEPSSolution operate on the deduplicated retained states, and _deduplicated_records keeps only the first record per Boolean state. Duplicate states' probability mass is therefore dropped rather than summed, and sample's cumulative-weight draw uses the un-renormalized retained probabilities. This is acceptable for "retained states" semantics but is a latent fidelity gap if two PEPS records decode to the same Boolean vector; a one-line comment or summing duplicates in prob would make the intent explicit.

Nonblocking: in TenSolver._solve_ising the per-transformation loop uses try ... finally clear_memoize_cache() with no catch. If any single transformation in transformations = :all throws inside SpinGlassEngine, the whole solve aborts even when other transformations would have succeeded. Issue #38 frames transformations as a set "to try"; consider catching and recording per-transform failures so one bad transform does not fail the solve. Not blocking since the default-path correctness is unaffected.

Nonblocking: the cutoff keyword (always injected by minimize(...; cutoff = 1e-8)) is silently absorbed by the extension's _solve_ising(...; cutoff = nothing, ...) and ignored. That is the right behavior for a DMRG-specific knob, but the extension's strict "Unsupported PEPS backend keyword(s)" guard relies on cutoff (and verbosity) being explicitly captured. A short comment noting that cutoff is intentionally accepted-and-ignored would prevent a future refactor from accidentally routing it into the kwargs guard and breaking minimize(Q; backend = PEPSBackend(...)).

3. Questions

Question: Closes #38. Issue #38's acceptance criteria require "A PEPS-backed direct API works for at least one small structured grid case on CPU" and "Exact small tests prove objective consistency." Because the SpinGlass stack does not resolve in the TenSolver environment (documented in the PR body and in docs/src/spinglasspeps_integration.md), that acceptance test is skipped, not passing. The core scaffolding, error handling, and DMRG-path solve_ising tests do pass. Given the real PEPS solve was never executed end-to-end, should this be Refs #38 (keeping #38 open until the component stack resolves and the gated test actually runs) rather than Closes #38? As written, merging would auto-close #38 while its primary acceptance criterion remains unverified.

Question: export Solution is added to src/TenSolver.jl. Solution was previously unexported (documented via TenSolver.Solution in api.md). Exporting it is a public-API surface change that is unrelated to the SpinGlassPEPS backend goal of this PR. Is this intentional and in scope for #44, or stack drift that belongs in a separate change?

4. Tests run and outcomes

Local execution blocked: julia and git were both denied by the review sandbox (confirmed even with sandbox disabled; not retried). Verified statically from head sources via gh pr diff/gh api contents. CI: all 12 check-runs success at head f3c4db1 (build, docs deploy, codecov patch+project, and the 9 Julia matrix jobs). The optional SpinGlassPEPS test is skipped in CI (Base.find_package returns nothing for the components), so the actual PEPS solve/decode path has no executed coverage anywhere.

5. Merge-readiness

Not mergeable yet, primarily due to the stacked-base dependency, not code defects. PR #44's base is feature/issue-37-backend-interface (PR #43, still OPEN), which itself chains through #42 -> #41 -> main. #44 cannot merge until #41, #42, and #43 merge first. Additionally, the whole #41..#46 stack was cut from old main (ca27880); current origin/main is 9a3dd5c (v0.2.0, with #49/#53/#54 merged), so the stack is stale and carries rebase/conflict risk against current main (Project.toml compat and exports are likely touch points). CI green here was computed on the stale base. No code changes are required from a correctness standpoint; the nonblocking items and the Closes #38 vs Refs #38 question above are the substantive follow-ups.

I would not merge this until the stacked base chain (#41-#43) merges and the stack is rebased onto current main; the Closes #38 linkage should also be reconsidered given the PEPS solve path is not exercised by CI.

[bernalde]

Pushed commits:

• c31f1e6 - merge updated PR Introduce solver backend interface #43 base into this stacked branch.
• edd04e4 - address PEPS backend review follow-ups.

Main changes:

• Resolved the Project.toml merge by keeping SpinGlass weakdeps/compat and the current QUBODrivers/QUBOTools compat from the updated base.
• Added an internal IsingModel adapter for the current QUBOTools-form conversion API so solve_ising and PEPS scaffolding still compile and test on the restacked branch.
• Removed the unrelated Solution export and added a regression check that Solution remains unexported.
• Changed the PEPS extension loop to record per-transformation failures and continue trying later transformations.
• Aggregated duplicate decoded-state probability mass in _deduplicated_records.
• Added/updated tests for the unexported surface, duplicate probability semantics, and restacked solve_ising path.

Tests run:

• git diff --check - passed.
• julia +1.12 --startup-file=no -e 'Meta.parse(read("ext/TenSolverSpinGlassPEPSExt.jl", String)); println("extension syntax parsed")' - passed.
• julia +1.12 --project=. -e 'using Pkg; Pkg.instantiate()' - passed.
• julia +1.12 --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/backend.jl"); include("test/peps_backend.jl")' - passed; backend interface 32 pass, PEPS backend core 37 pass, optional SpinGlassPEPS extension 1 expected broken.
• julia +1.12 --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/ising_conversion.jl")' - passed, 3611 tests.
• 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.

Comments intentionally not addressed:

• 3376275944: no code change to raise TenSolver's Julia compat or activate the SpinGlass stack. This remains experimental scaffolding allowed by [SpinGlassPEPS 4/6] Add optional SpinGlassPEPS-backed structured solver path #38; follow-up is still needed to exercise the extension under Julia 1.11 or once the SpinGlass compat stack resolves.
• Codecov coverage comment: not addressed in this PR because the real extension path remains gated by optional dependencies that default CI cannot currently resolve.

Remaining risks or follow-up:

• GitHub CI is pending for the pushed branch.
• The real SpinGlassPEPS solve path remains unexercised in default CI.
• Downstream stacked PRs Add JuMP PEPS backend attributes #45 and Add PEPS docs, examples, and benchmarks #46 need to merge this updated Add optional SpinGlassPEPS backend #44 base before their review comments are revisited.

[bernalde]

Commits pushed:

• 98a3573 Clarify PEPS scaffolding issue scope.

Main changes made:

• Documented that this stack step is internal PEPS scaffolding, not final completion of [SpinGlassPEPS 4/6] Add optional SpinGlassPEPS-backed structured solver path #38.
• Updated the implementation plan wording so the issue remains open until a real small structured-grid CPU solve runs through the optional SpinGlass component stack.
• Updated the PR body from Closes #38 to Refs #38 and recorded the same activation condition there.

Tests run and results:

• git diff --check: passed.
• julia +1.12 --project=docs/ docs/make.jl: passed.
• julia +1.12 --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/utils.jl"); include("test/backend.jl"); include("test/peps_backend.jl")': passed; backend interface 32/32, PEPS backend core 37/37, optional SpinGlassPEPS extension 1 expected broken.
• julia +1.12 --project=. -e 'using Pkg; Pkg.test(; coverage=false)': passed.
• gh pr checks 44 --repo SECQUOIA/TenSolver.jl --watch --fail-fast: passed across docs and the full Julia lts/1/pre matrix on Ubuntu, macOS, and Windows.

Comments intentionally not addressed:

• No Blocking comments were declined.
• I did not raise TenSolver's Julia compat or force activation of the SpinGlass stack in this PR. The extension remains experimental scaffolding until the optional component stack can resolve and run in CI.
• Codecov's extension coverage warning remains expected because the real optional extension path is still gated by unavailable weak dependencies.

Remaining risks or follow-up items:

• The real SpinGlassPEPS solve/decode path is still unexercised in default CI.
• [SpinGlassPEPS 4/6] Add optional SpinGlassPEPS-backed structured solver path #38 should remain open until a follow-up provides a passing small structured-grid CPU solve through the real optional SpinGlass component stack.

[iagoleal]

General comments:

• we should try to keep the API as unified as possible.
  • minimize / maximize or JuMP as only solver entry-points;
  • PEPSBackend should be similar to DMRG;
  • There should be an abstract Solution type with a clear interface and each backend may then specialize to a subtype of its own. (Or just a general Solution type that is general enough to work for all backends)
  • Let's keep the file structure organized: solver.jl for general solver inteface, backend/*.jl for the backends properly.
  • Only export methods that the user should see. Keep internal helpers internal.
• I like that most of the hard word is being done at an extension level.
• For easier autocomplete / style, do not start function names with an underline.
• With the latest rebase, some stuff got duplicated and some other broke. We have to investigate how to properly update this PEPS code to align it with the current main branch.

I still didn't have the time to deep dive into the backend code itself. But these point should already get things running.

[iagoleal]

This file must be merged with docs/internal/spinglasspeps_integration.md. During the branch rebasing, we ended up with both of them.

[bernalde]

Merged. The docs/src copy was the newer one (post-#43 content), so its content now lives at docs/internal/spinglasspeps_integration.md and the docs/src duplicate is gone, along with its entries in docs/make.jl and docs/DOCUMENTATION.md — the design note is internal-only again.

[bernalde]

Still addressed on the current head: there is one design note at docs/internal/spinglasspeps_integration.md, and the duplicate docs/src copy remains removed.

[bernalde]

Still addressed at current head 33764cb: the single SpinGlassPEPS design note remains at docs/internal/spinglasspeps_integration.md, with no duplicate under docs/src.

[iagoleal]

Suggested change

- **SpinGlassPEPS Integration** (`docs/src/spinglasspeps_integration.md`): Planned architecture for the optional structured PEPS backend

Internal

[bernalde]

Applied — removed from the public docs listing; the internal-notes entry below already covers it.

[bernalde]

Still addressed on the current head: the SpinGlassPEPS design note is not listed as public documentation.

[bernalde]

Still addressed at current head 33764cb: the SpinGlassPEPS design note remains absent from the public documentation listing.

[iagoleal]

Suggested change

"SpinGlassPEPS Integration" => "spinglasspeps_integration.md",

Internal

[bernalde]

Applied — removed from the Documenter pages; the design note is internal-only.

[bernalde]

Still addressed on the current head: the internal design note is absent from the Documenter page navigation.

[bernalde]

Still addressed at current head 33764cb: the internal SpinGlassPEPS design note remains absent from Documenter navigation.

[iagoleal]

Suggested change

ising_to_qubo(model::IsingModel) = ising_to_qubo(ising_form(model.J, model.h, model.offset))

Leftover from rebasing.

[bernalde]

Addressed, though not by deleting this line — the actual rebase leftover was more serious and one level up: the rebase lost the IsingModel struct and its validating constructor entirely (which is why the package stopped precompiling and CI went red), and left src/ising.jl included twice in TenSolver.jl (which duplicated every docstring and broke the Documentation build). This method itself is required — solve_ising(::DMRGBackend, model) and the PEPS extension both convert through ising_to_qubo(model).

Fixed by restoring the struct/constructor (with the diagonal-coupling folding) and dropping the duplicate include (f244b53), plus restoring the IsingModel(::QUBOTools.AbstractForm) form constructor and scaled_form_parts (8459f8d). The solve_ising preserves pair couplings and offsets regression test covers the restored behavior.

[iagoleal]

The IsingModel type was deleted as part of the final review in #42, but the changes never reached this branch. I'm ok with either deleting or keeping it, but we should reaccess why that PR chose to not provide this type.

[bernalde]

Reassessed against current main and removed the obsolete IsingModel type and its adapter methods. PEPS now consumes native spin-domain inputs through minimize(...; domain = [-1, 1]).

[bernalde]

Still addressed at current head 33764cb: the obsolete IsingModel type and adapters remain removed, and PEPS consumes native spin-domain inputs through minimize(...; domain = [-1, 1]).

[iagoleal]

We should strive for keyword compatibility with the DMRG backend. When a concept applies to both, we should use the same name.
Ex: onGPU vs device, bond_dim vs maxdim, num_sweeps vs iterations, etc.

Also, why is the DMRGBackend "empty" with all keywords being passed to minimize while PEPSBackend has many fields? Let's keep it consistent.

[bernalde]

Done (commit ea75764), both halves:

• PEPSBackend is now "empty" like DMRGBackend — it carries only the topology (structural, not tunable). All solver parameters moved to keywords of the solve call.
• Shared concepts reuse the DMRG names: maxdim (was bond_dim), iterations (was num_sweeps), device (was onGPU; any non-cpu device runs the contractor on GPU). The PEPS-specific keywords (beta, max_states, cutoff_prob, contraction, graduate_truncation, transformations, local_dimension, no_cache) are normalized and validated by a new TenSolver.peps_options helper, which the extension consumes — so keyword validation is testable without the optional SpinGlass stack.

[bernalde]

Addressed on the current design: PEPSBackend carries only its structural topology, shared concepts use DMRG-compatible names such as maxdim, iterations, and device, and PEPS-only option normalization is internal to the extension.

[bernalde]

Still addressed at current head 33764cb: PEPSBackend stores only structural topology, shared concepts use DMRG-compatible keyword names, and PEPS-only option normalization remains internal to the extension.

[iagoleal]

Suggested change

	preprocess && throw(ArgumentError("PEPSBackend does not support preprocess=true because the topology fixes the variable order. Use backend = :dmrg for preprocessed QUBO solves."))
	preprocess && throw(ArgumentError("PEPSBackend does not support preprocess=true because the topology fixes the variable order.

Separation of concerns. What if we add more backends supporting preprocessing?

[bernalde]

Applied (commit b5615ac) — the message now states only this backend's constraint, without steering to a specific alternative backend.

[bernalde]

Still addressed: the preprocessing error describes only PEPSBackend's topology constraint and does not direct users to or couple it with another backend.

[bernalde]

Still addressed at current head 33764cb: the preprocessing error describes only the PEPSBackend topology constraint and does not couple the message to another backend.

[iagoleal]

Suggested change

	export minimize, maximize, solve_ising
	export minimize, maximize

Let's keep the API simple.

[bernalde]

Applied (commit 17207ec) — solve_ising is no longer exported; minimize/maximize (and JuMP) are the solver entry points. It stays documented under "Internal Functions" in the API reference as the backend boundary that structured backends implement, and the tests now call it as TenSolver.solve_ising.

[bernalde]

Addressed more completely on the current head: solve_ising has been removed rather than merely unexported. minimize/maximize and JuMP remain the solver entry points.

[bernalde]

Still addressed at current head 33764cb: solve_ising remains removed, leaving minimize, maximize, and JuMP as solver entry points.

[iagoleal]

Suggested change

	include("ising.jl")
	export bool_to_spin, spin_to_bool, qubo_to_ising, ising_to_qubo

Beware to not include this twice.

[bernalde]

Applied (commit f244b53) — the duplicated include("ising.jl") + exports block is gone; the file is included exactly once. This double include was also what broke the Documentation build via duplicate docstrings.

[bernalde]

Still addressed: src/ising.jl is included exactly once.

[bernalde]

Still addressed at current head 33764cb: src/ising.jl remains included exactly once.

[iagoleal]

These test are not about the backend infrastructure. They should be moved to the peps_backend tests.

[bernalde]

Done (commit 17207ec) — the solve_ising preserves pair couplings and offsets regression testset moved from test/backend.jl into test/peps_backend.jl, next to the rest of the Ising-boundary coverage; test/backend.jl is back to pure backend-interface tests.

[bernalde]

Addressed on the current test layout: test/backend.jl contains backend-interface coverage, while PEPS-specific behavior is covered in test/peps_backend.jl. The obsolete solve_ising tests were removed with that API.

[bernalde]

Still addressed at current head 33764cb: backend-interface tests remain in test/backend.jl, and PEPS-specific behavior remains in test/peps_backend.jl.

[bernalde]

Addressed the CHANGES_REQUESTED review. Eight commits pushed (head 8459f8d):

1. f244b53 — Restore IsingModel struct lost in rebase; drop duplicated ising include. This was the root cause of the red CI on 673cd63: the rebase dropped the struct/constructor while call sites still referenced it (package no longer precompiled), and the doubled include("ising.jl") duplicated every docstring (Documentation build failure).
2. ca15a91 — Merge duplicated SpinGlassPEPS design doc into docs/internal (kept the newer content) and remove it from the public docs nav (docs/make.jl, docs/DOCUMENTATION.md).
3. b5615ac — Move all PEPS code to src/backends/peps.jl, mirroring the DMRG layout; drop every underscore-prefixed helper name (solver + extension); restore backend_error(::PEPSBackend) (also lost in the rebase); trim the preprocess error message per suggestion.
4. 3879729 — Introduce abstract type AbstractSolution documenting the sample/prob interface, with generic sample(psi, n) and in(xs, psi; cutoff); Solution and PEPSSolution subtype it.
5. abd0720 — Enforce unique states in PEPSSolution (the extension already merges duplicates; the inner constructor now guarantees it) and simplify prob to a lookup.
6. ea75764 — Unify PEPS solve keywords with DMRG conventions: PEPSBackend now carries only the topology (like DMRGBackend); maxdim/iterations/device replace bond_dim/num_sweeps/onGPU; PEPS-specific keywords are validated by the new TenSolver.peps_options, consumed by the extension.
7. 17207ec — Keep solve_ising unexported (minimize/maximize/JuMP stay the entry points), reorder the backends docs block, document AbstractSolution + PEPS keywords, and move the solve_ising regression tests into the PEPS backend suite.
8. 8459f8d — Restore the IsingModel(::QUBOTools.AbstractForm) constructor and scaled_form_parts (also dropped by the rebase, which broke solve_ising(::DMRGBackend, model)), and give SquareGrid/KingGrid the docstrings their api.md entries expect.

Review-body findings: API unification → commits 4, 6, 7; file structure → commit 3; export hygiene → commit 7; no leading underscores → commit 3; rebase duplication/breakage → commits 1, 2, 8.

Not changed: ising_to_qubo(model::IsingModel) was kept (the inline suggestion proposed deleting the adjacent line) — it is required by solve_ising(::DMRGBackend, ...) and the extension; the real rebase leftover was the lost struct + doubled include, fixed in commits 1 and 8. Details in the inline reply.

Tests: full Pkg.test() suite green locally on Julia 1.10 (0 failures); PEPS backend core + the moved solve_ising regression testset pass.

Docs build: docs/make.jl builds clean — no missing/duplicate @docs entries; the duplicated design doc and doubled include that broke it before are gone.

CI on 8459f8d: build, documenter/deploy, and the lts/1 (stable) test jobs on Linux/macOS/Windows all pass. The Julia pre - windows job failed at precompile with ArgumentError: Package Combinatorics ... is required but does not seem to be installed — a Julia 1.13-prerelease + Windows package-materialization flake, not a code issue (Combinatorics is a declared dep; the pre - macOS and both stable Windows jobs on this same head are green, as is the full local Julia 1.10 suite). Re-running that job to confirm; it's not a required check.

Formal gate: the review decision remains CHANGES_REQUESTED until @iagoleal re-reviews or dismisses it. The extension solve path also remains CI-gated (Julia ≥ 1.11 + upstream euro-hpc-pl/SpinGlassNetworks.jl#23), unchanged from before.

[bernalde]

CI follow-up: the Julia pre - windows failure reproduced identically on re-run — it is not a transient flake but a persistent Julia 1.13-prerelease + Windows toolchain issue. Both runs fail at precompile with Missing source file for ... Combinatorics even though the package resolves and installs (+ Combinatorics v1.1.0 appears in the log). Combinatorics is a long-standing declared dependency, untouched by this PR.

Not caused by these changes: every stable (1) and lts job on Linux/macOS/Windows passes, pre - macOS passes, and build + documenter/deploy are green; only pre - windows is red. The last pre-rebase green run of this branch also passed pre - windows, so this is upstream prerelease-toolchain drift, not a regression here.

Since main isn't branch-protected and pre isn't a required check, it doesn't gate merge. Orthogonal suggestion for a separate change: mark the pre matrix row continue-on-error: true so prerelease-toolchain breakage stops reddening the whole CI matrix.

[iagoleal]

A lot has happened in the repo since this PR's last review, so here go a lot of comments. It is a lot, so we will probably have to do a couple iterations more before merging.

The most important thing is that we cannot merge this with a red CI. Right now, it is just not compatible with our package.

Some minor design comments:

• After merging main, it is worth passing JuliaFormatter on the new ext/TenSolverSpinGlassPEPSExt.jl and src/backends/peps.jl files;
• Now that we natively support ising domains, do we need to keep the elpers in ising.jl? The user will supposedly pass an Ising model directly to minimize
• Keep the minimize / maximize API as the only solver entry-points.
• In terms of syntax and "expected behaviour", align the DMRG and PEPS backends as much as possible. For example, preferably there should be a single solution API for both without the user accessing different fields.

[iagoleal]

In Julia, having a try-catch block inside a tight loop is terrible for performance.

[bernalde]

Addressed: the loop no longer contains a try/catch. The transformation helper uses only try/finally so the memoization cache is cleared while any failure still propagates.

[iagoleal]

try/finally is equally slow, so we should make sure it is not called from a tight loop. Legit question: Why do we assume the code can fail?

[bernalde]

Addressed in 33764cb: removed try/finally from the per-transformation solve path. Construction and spectrum failures now propagate directly; the memoized SpinGlass contraction state is cleared only after a successful low_energy_spectrum call, matching SpinGlassEngine 1.6 benchmark/test usage. The extension is JuliaFormatter-clean, the full package suite and docs build pass locally, and all current-head CI checks are green.

[bernalde]

Addressed the requested review pass on the current base and updated the branch to current main.

Commits pushed:

• 90b7de2 — merge current main into the PEPS backend branch without rewriting its history.

Main changes and review verification:

• The base merge was conflict-free and leaves PR Add optional SpinGlassPEPS backend #44 zero commits behind main.
• Rechecked all 17 unresolved inline threads against the merged head. Their requested changes remain present: the obsolete IsingModel/solve_ising APIs are absent; Solution is the shared abstract interface with DMRGSolution and PEPSSolution; PEPS helpers have no leading underscores; backend keywords follow the DMRG naming where concepts overlap; the design note is internal-only; and the per-transformation hot path has no try/catch or try/finally frame.
• No additional source fix or empty commit was needed beyond the base merge. Existing marker-bearing replies already cover every unresolved thread, and no newer thread comment superseded them.

Verification:

• git diff --check origin/main...HEAD — passed.
• Targeted backend/PEPS command — 21 backend-interface checks and 39 PEPS-core checks passed; the real optional SpinGlassPEPS extension remains one expected skip because the optional stack is unavailable.
• julia --project=. --startup-file=no -e 'using Pkg; Pkg.test(; coverage=false)' — passed on Julia 1.10.11, including Ising conversion, projection, constrained solving, QUBODrivers, and Aqua.
• julia --project=docs --startup-file=no docs/make.jl — passed on Julia 1.10.11; deployment was skipped outside CI.
• The PEPS source files changed by the review were not modified by the base merge; their previously validated JuliaFormatter output remains unchanged. JuliaFormatter is not installed in the current local environment, so it was not redundantly reinstalled for this no-source-change pass.

Comments intentionally not addressed:

• No Blocking review comment was declined.
• The registered SpinGlass component stack still cannot exercise the real structured-grid CPU solve in the default environment. This remains follow-up work under [SpinGlassPEPS 4/6] Add optional SpinGlassPEPS-backed structured solver path #38, and the PR continues to use Refs #38.

Remaining gates:

• Current-head GitHub Actions are running for 90b7de2.
• The formal review decision remains CHANGES_REQUESTED until Iago rereviews or dismisses the prior decision.
• Review threads were not marked resolved.

[bernalde]

Merged current main at 5b49f2b (per-variable domains, #131) in c88e2f5, resolving the conflicts and adapting the branch to the new domain API:

• src/solution.jl keeps this PR's abstract Solution interface and SolverStatistics; the concrete type stays in the DMRG backend.
• DMRGSolution absorbs main's per-variable semantics: its domain field is now Domains{T}, sample maps each site through its own domain with the non-mutating sampler, and coeff validates through in/searchsortedfirst with the corrected $(repr(...)) interpolation.
• minimize_mpo computes the sampled objective in tensor order (main's form) and constructs a DMRGSolution.
• The SpinGlassPEPS extension now takes the Domains type: the spin check requires every per-variable domain to equal [-1, 1], and the solve path explicitly rejects native constraints, since the front door now always forwards a constraints vector (previously that keyword would have crashed peps_options).
• docs/src/api.md carries both the Variable Domains and PEPS Backend sections.

Verification at c88e2f5: full Pkg.test passes locally on Julia 1.12.0 (PEPS backend core 39/39; the optional-extension testset still skips without the SpinGlass stack, as designed). The extension source parses standalone, and the DMRG spin-domain solve, sampling, and probability round-trip were smoke-checked directly. The known gap from the PR body is unchanged: the real SpinGlassPEPS CPU solve still needs the optional stack, so #38 stays Refs.