Add Q-matrix preprocessing permutation#33
Conversation
There was a problem hiding this comment.
No blocking issues found. I inspected the PR diff, surrounding solver/Solution/JuMP code, tests, docs, README, and CI workflows. The implementation keeps caller-facing bit order stable while using the permuted tensor order internally, and the one-variable fallback is consistent with the existing API surface.
Tests run locally:
- julia +1.12 --project=. -e 'using Pkg; Pkg.test()' passed.
- julia +1.12 --project=docs/ docs/make.jl passed.
- julia +1.12 --project=test -e 'push!(LOAD_PATH, pwd()); using Test, TenSolver, JuMP, LinearAlgebra; ...' targeted preprocessing/JuMP/one-variable smoke passed.
bernalde
left a comment
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: merge-base ca27880; git diff <merge-base>..HEAD --stat = 6 files, +228 / -5, matching the PR's reported totals.
Issue intent (#12): the PR implements a real heuristic, not a placeholder — a weighted Reverse Cuthill-McKee ordering (src/solver.jl) that reduces coupling bandwidth so the QUBO better matches a 1D MPS chain. Verified behaviorally: on a scrambled path graph it reduces bandwidth 3 -> 1. This is a reasonable interpretation of #12.
Correctness (the critical check) — apply/invert is correct:
- Apply:
preprocess_quboreturnsQ[perm, perm],l[perm]; sitek= original variableperm[k]. - Invert on sampling:
original_orderdoesx[perm] = bs(src/solution.jl), mapping site-order samples back to original variable order. - Invert on lookup:
coeffdoesbs = bs[psi.permutation], the correct inverse direction. - Energy uses the original
Q/lon the already-un-permuted sample, so returned energy is in the original problem space. - End-to-end check:
minimize(Q, l; preprocess=true)returned the correct energy and a sample in original variable order. No un-inverted-permutation bug.
Default behavior unchanged: preprocess defaults to false in both the JuMP attribute and the minimize kwarg; existing results are not altered unless a user opts in. maximize forwards the kwarg too.
Nonblocking
qmatrix_permutationis exported indocs/src/api.md@docsbut its docstring does not document thecutoffargument, the return-value semantics (entrykis the original index for sitek), or include an example. Expand with an Arguments/Returns section and a smalljldoctest.- Cutoff default inconsistency: standalone
qmatrix_permutationdefaultscutoff = 0(src/solver.jl:102) butpreprocess_qubopasses the solver'scutoff = 1e-8. Harmless, but add a one-line comment noting the two callers' differing thresholds are intentional.
Questions
3. Completeness of Closes #12: RCM is a single static heuristic; #12 frames the ordering problem as NP and references specific literature (the issue comment points to the tridiagonal-QUBO-in-P result, arXiv:2309.10509). RCM is a fine first implementation but does not try alternatives or claim optimality. Consider Refs #12 and keeping the issue open for follow-up, or accept RCM as sufficient — reviewer's call.
4. The PR bundles an exact one-variable solve path (src/solver.jl, length(sites)==1 branch) that is logically independent of permutation. It is correct and covered by the new "One variable" test, but is the same single-variable fix appearing across several parallel PRs (#34, #41) — worth de-duplicating across the open PRs and calling out in the description.
5. The PR description claims Pkg.test() failed on a manifest/OpenSSL_jll mismatch requiring julia +1.12. On Julia 1.10.11 in a clean worktree, Pkg.instantiate() and the full Pkg.test() both succeeded — that caveat appears to be a local-environment artifact and can be dropped.
Tests run (Julia 1.10.11, clean worktree on the PR head):
julia --project=. -e 'using Pkg; Pkg.instantiate(); Pkg.test()'— full suite passed (Testing TenSolver tests passed): QUBO Correctness 78/78 incl. new "One variable", "Permutation reduces coupling bandwidth", "Samples are returned in original variable order"; JuMP preprocess attribute 2/2; QUBODrivers.jl 130; Aqua.jl 11; VRP 2; HDF5 16; Doctests 1.- Independent round-trip: permutation validity, bandwidth 3 -> 1,
preprocess=truevsfalseagree on energy.
Merge readiness: Ready to merge after minor polish. The core risk — a permutation not inverted on output — is not present; apply and invert are correct and verified. Default behavior is unchanged (opt-in), and the full suite passes. Address the two docstring/cutoff nonblocking items and decide Closes vs Refs #12. The formal verdict must come from another maintainer.
| Return a deterministic permutation that places coupled QUBO variables closer | ||
| together in the one-dimensional MPS ordering. | ||
| """ | ||
| function qmatrix_permutation(Q::AbstractMatrix; cutoff = 0) |
There was a problem hiding this comment.
Nonblocking: qmatrix_permutation is exported in the docs @docs block but its docstring omits the cutoff argument, the return-value semantics (entry k is the original index for site k), and an example. Also note the default here is cutoff = 0 while preprocess_qubo calls it with cutoff = 1e-8 — worth a comment that the differing thresholds are intentional.
There was a problem hiding this comment.
Addressed in 2c9cf21. The qmatrix_permutation docstring now documents cutoff, return semantics, an example, and the intentional 0 vs 1e-8 cutoff difference.
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Senior maintainer review of PR #33 (Add Q-matrix preprocessing permutation). Diff totals confirmed against the merge-base via gh: +228 / -5 across 6 files, matching the PR's reported totals. The head was reviewed statically from the exact head SHA (554fe7a); full head file contents were retrieved via the GitHub contents API.
Procedural note: I am the PR author identity, so GitHub will not let me APPROVE or REQUEST_CHANGES on my own PR. This is posted as event=COMMENT. The formal verdict must come from another maintainer.
Environment limitation (disclosed for transparency): in this review sandbox, git and julia invocations were denied, so I could not check out the head working tree or run the test suite myself. I therefore could not independently reproduce the PR's claimed Pkg.test() / docs-build passes, and could not re-run the new testsets. All findings below are from static analysis of the diff plus the full head sources; the test-execution claims in the PR description remain unverified by me.
--- Blocking ---
-
The PR conflicts with
mainand re-implements one-variable QUBO handling thatmainalready provides.gh pr view 33reports mergeable=CONFLICTING, mergeStateStatus=DIRTY. The PR branch (commits dated 2026-05-15) was cut before #49 (fix: solve single-variable QUBO inputs exactly instead of crashing, merged 2026-06-11, commit b67de66) landed onmain.main's_minimizealready hasif length(sites) == 1; return _single_variable_minimize(...), whereas this PR adds a different inlineif length(sites) == 1 ... endblock in the same region (src/solver.jl head lines 324-343). These two implementations collide. Why it matters: the PR cannot merge as-is, and the bundled one-variable code is now redundant work that duplicates and diverges from the merged solution. Fix: rebase onto currentmain, drop this PR's inline one-variable block and thetest/qubo.jl"One variable" testset (or reconcile them with_single_variable_minimize), and keep only the permutation feature, which is the actual subject of issue #12. Refs #33, and main commit b67de66. -
Closes #12overstates and mixes scope. Issue #12 asks only for Q-matrix permutation toward a more 1D (tridiagonal) site ordering. This PR delivers that, but also bundles unrelated one-variable solve handling (item 1). A faithful resolution of #12 is the permutation work alone. Fix: scope the PR to the permutation feature and keepCloses #12; remove the one-variable changes (already solved on main). If for some reason one-variable code must stay, it should beRefs, notCloses, and reconciled with the existing path. Refs #12. -
Tests for the changed behavior were not verifiable here, and the suite cannot be trusted from the PR description alone. I could not run
julia +1.12 --project=. -e 'using Pkg; Pkg.instantiate(); Pkg.test()'(julia execution denied in this environment). Because of the merge conflict in item 1, the suite as it would land onmaindoes not even exist as a clean tree yet. Another maintainer must run the full suite on a clean rebased checkout before merge and confirm both the new permutation testsets and the existing single-variable tests pass together. This is blocking only in the sense that changed behavior must be shown green on a mergeable tree; it is not a claim that the code is wrong.
--- Nonblocking ---
-
qmatrix_adjacencyiterates all O(n^2) index pairs ofQeven whenQis sparse (the JuMP/QUBODrivers path passes a sparse matrix fromQUBOTools.qubo(...; :sparse)). For a preprocessing pass this is acceptable, but it defeats sparsity for large problems. Consider iterating stored nonzeros for the sparse case. src/solver.jlqmatrix_adjacency. -
qmatrix_permutationdefault iscutoff = 0, whilepreprocess_quboforwards the solvercutoff(default 1e-8). The two defaults differ; this is fine in practice since the solver always passes its own cutoff, but the public docstring should state that edges withabs(Q[i,j]+Q[j,i]) <= cutoffare dropped, so users callingqmatrix_permutationdirectly understand the thresholding. src/solver.jlqmatrix_permutationdocstring.
--- Questions ---
-
In the inline one-variable block, the degenerate case (both bit values optimal) uses the
"full"state label and bond_dim is logged as 1. Issampleover the"full"Qudit state guaranteed to return only 0/1 product bitstrings with the intended uniform probability, and is[x] in psi(viacoeff) correct for both branches? I could not run this. Given item 1, the question may be moot if this block is removed in favor of_single_variable_minimize; if it is kept, please confirm behavior with a test that exercises the degenerate (["full"]) path. src/solver.jl head lines 324-343. -
The permutation convention (site s holds original variable
permutation[s];original_orderdoesx[permutation] = bs;coeffdoesbs[permutation]) is internally consistent under static reading. Has this round-trip been validated against a non-identity permutation where the optimum is asymmetric, beyond the included 3-variable symmetric test, to guard against an inverse-permutation mistake? src/solution.jloriginal_order/coeff.
--- Summary ---
- Blocking: (a) PR conflicts with main and duplicates/diverges from the already-merged one-variable path (#49); (b)
Closes #12bundles unrelated one-variable scope; (c) changed behavior not shown green on a clean, mergeable tree. - Nonblocking: dense O(n^2) adjacency scan over sparse Q; document cutoff thresholding in
qmatrix_permutation. - Questions: correctness of the degenerate
"full"one-variable branch; permutation round-trip coverage for asymmetric/non-identity cases. - Tests run and outcomes: none executed by me;
gitandjuliawere denied in this review environment, so the head tree could not be checked out and the suite could not be run. The PR-reported passes are unverified. Diff totals were verified viagh(+228/-5, 6 files) and the head sources were read via the contents API. - Merge-readiness: not ready. The PR is in a CONFLICTING state and must be rebased onto current
main, with the one-variable code removed/reconciled and the suite shown green, before it can be considered. The permutation feature itself appears sound on static reading.
I would not merge this until the blocking issues above are addressed.
| # Quantization | ||
| sites = ITensorMPS.siteinds(first, H; plev=0) | ||
|
|
||
| if length(sites) == 1 |
There was a problem hiding this comment.
Blocking. This inline one-variable block collides with main, which already returns _single_variable_minimize(...) from the same if length(sites) == 1 point (added by #49, commit b67de66, merged 2026-06-11). gh pr view 33 reports CONFLICTING/DIRTY. Rebase onto current main and drop this block (and the corresponding test/qubo.jl "One variable" testset), keeping only the permutation feature that issue #12 asks for. If kept, reconcile it with _single_variable_minimize rather than duplicating it.
There was a problem hiding this comment.
Addressed in 2c9cf21. The branch was brought current with main, and the duplicate one-variable block plus weaker testset were dropped in favor of the existing _single_variable_minimize path and tests.
| labels = length(states) == 2 ? ["full"] : string.(states) | ||
| psi = MPS(sites, labels) | ||
| elapsed_time = time() - initial_time | ||
| dist = Solution{T}(psi, T[optimal], Int[1], Float64[elapsed_time], permutation) |
There was a problem hiding this comment.
Question. The degenerate branch uses the "full" state label when both bit values are optimal. Please confirm that sampling and the coeff/in check behave correctly over the "full" Qudit state (uniform over {0,1}), ideally with a test exercising this path. Note this may be moot if the block is removed per the blocking comment above.
There was a problem hiding this comment.
Not applicable after 2c9cf21. The questioned branch-local one-variable block was removed; the existing main single-variable degeneracy test remains and passed in Pkg.test().
| function minimize(Q :: AbstractMatrix{T} , l :: Union{AbstractVector{T}, Nothing} = nothing , c :: T = zero(T); cutoff=1e-8, kwargs...) where T | ||
| H = tensorize(Q, isnothing(l) ? diag(Q) : diag(Q) + l; cutoff) | ||
| function minimize(Q :: AbstractMatrix{T} , l :: Union{AbstractVector{T}, Nothing} = nothing , c :: T = zero(T); cutoff=1e-8, preprocess::Bool=false, kwargs...) where T | ||
| Qp, lp, permutation = preprocess ? preprocess_qubo(Q, l, cutoff) : (Q, l, nothing) |
There was a problem hiding this comment.
Permutation wiring looks consistent on static reading: obj keeps the original Q/l, samples are unpermuted before obj is applied, and H is built from the permuted Qp/lp. Nonblocking: qmatrix_adjacency scans all O(n^2) pairs even though the QUBODrivers path passes a sparse Q; consider iterating stored nonzeros for large sparse inputs.
There was a problem hiding this comment.
Addressed in 2c9cf21. qmatrix_adjacency now builds candidate edges from nonzero matrix entries instead of scanning all pairs, and test/qubo.jl covers asymmetric/cutoff behavior.
|
Review comments addressed in Commits pushed:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 96.89% 98.13% +1.24%
==========================================
Files 4 4
Lines 193 268 +75
==========================================
+ Hits 187 263 +76
+ Misses 6 5 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
Solutionsamples and probability checks.preprocessoptimizer attribute through the QUBODrivers/JuMP solve path.maininto this branch and drop the stale duplicate one-variable QUBO handling that already landed via minimize/maximize crash on single-variable (1x1) QUBO inputs #49.qmatrix_permutationdocumentation forcutoff, return semantics, and examples.Tests run
git diff --check origin/main...HEADjulia +1.12 --project=. -e 'using Pkg; Pkg.test()'julia +1.12 --project=docs/ docs/make.jljulia +1.12 --project=docs/ -e 'using Pkg; Pkg.instantiate()'julia +1.12 --project=docs/ docs/make.jlCloses #12