Design JuMP/MOI constrained optimization integration - #103
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Maintainer review — design-only PR for #65
Docs-only change: adds docs/internal/jump_moi_constraints.md (+358) and three lines to docs/DOCUMENTATION.md. No code, no dependencies. I authored this PR, so it posts as COMMENT (GitHub rejects author APPROVE); main is not branch-protected and GitHub reports no review gate, so no formal approval is enforced — but a second maintainer's read is worthwhile for a design that will steer an implementation epic.
Acceptance criteria (#65) — all met
- Specific enough for a follow-up epic: yes — a staged 5-part implementation plan plus a design acceptance checklist.
- Separates native Julia API from MOI bridge: yes — "Current Boundary" and "Public API Decision".
- Native types as lowering targets for MOI sets/bridges, incl. point-on-set: yes — "Point-on-Set Versus Projection MPOs" correctly distinguishes
distance_to_set/MathOptSetDistances (numeric point projection) from TenSolver's Hilbert-space diagonal projectorP, and declines the extra dependency. - Lists unsupported constraints + fallback: yes — explicit hard-error policy, no implicit penalty-QUBO or generic-enumeration fallback.
- References CoTenN: yes.
Verified technical claims
Checked the load-bearing claims against the resolved dependency versions rather than taking them on faith:
- QUBODrivers status override — correct and load-bearing. Against QUBODrivers v0.6.5, the generic
AbstractSamplergetterMOI.get(_, ::MOI.TerminationStatus)(src/library/sampler/wrappers/moi.jl:477) returnsMOI.OTHER_ERRORfor an empty sample set with nonempty metadata and does not readmetadata["termination_status"]. So the doc's instruction to add a concreteTerminationStatusoverride is necessary, not optional — and it mirrors what QUBODrivers' ownMIPSamplerdriver does (readsmetadata["termination_status"]first). Good to have this pinned in the design. - Mapping-table rows all check out:
sum(x)==n-1→ExactlyOne(·,0);x_i+x_j<=1→NotEquals([i,j],[1,1]);AllDifferent(2)→Relation(!=)with >2 binary statically infeasible;SOS1→sum<=1(bond 3, correctly "at most one", all-zero feasible so not exactly-one); thex_i−x_jrelation rows; and the nonnegative-integer canonicalization (sign-flip reverses<=/>=, mixed-sign rejected unless pairwise).RelationConstraintdoes accept:(!=)(src/constraints.jl). - Infeasibility status contract matches the landed #97 behavior and
tensolver_statusreturningMOI.INFEASIBLE.
Findings
Two Nonblocking notes inline; no blocking issues.
Tests / CI
Docs-only. CI on ec343ab is green across all 10 checks (build, Documentation, and the 9-way Julia matrix). The note lives under docs/internal/ and is not in docs/make.jl's page list, so Documenter does not process it — consistent with the existing spinglasspeps_integration.md.
Merge-readiness
No blocking findings; the two inline notes are optional polish for the follow-up epic. One coordination item (already called out in the PR description): PR #46 also appends to the internal-notes list in docs/DOCUMENTATION.md; the edits are disjoint, so whichever lands second needs a one-line resync. This account cannot post a formal APPROVE on its own PR, and no approval gate is reported as required.
|
Addressed both nonblocking review notes in Changes:
Verification:
No review comments were intentionally left unaddressed. The two threads remain unresolved for reviewer confirmation; there is no formal |
iagoleal
left a comment
There was a problem hiding this comment.
Its mostly good to go. I've left some comments.
The most important point to keep in mind:
- This design document should coordinate/take into account the changes in #44 (multiple backends), #105 (non-binary data) and #68 (polynomial objectives via JuMP). Even if some are not implemented here, the interface should be extensible enough to account for those changes. In particular, #105 already landed.
|
Addressed the current review feedback and refreshed PR 103 against Commits pushed:
Main changes:
Verification:
No actionable comments were intentionally left unaddressed. The positive informational thread required no code change and is acknowledged separately. The formal review decision remains |
Summary
TenSolver.Optimizeras the single JuMP entry point, backed by a TenSolver-owned full MOI model store rather than a Boolean-QUBO-only copy pathThis PR is design-only. It does not add functional JuMP constraint support or new dependencies.
Key decisions
AbstractConstraintsubtypes remain the compact, projection-aware lowering targets.ZeroOne, quadratic-objective, or DMRG assumption. The first backend adapter targets DMRG's current uniform finite-domain contract and leaves heterogeneous domains to Epic: DMRG optimization over non-Boolean domains #67.SumConstraintwith an interval relation and one capped DFA instead of introducing a redundant constraint type.MOI.INFEASIBLEwith zero results and no primal values.Coordination
Validation
julia +release --startup-file=no -e 'using Markdown; for file in ARGS; Markdown.parse(read(file, String)); println("parsed ", file); end' docs/internal/jump_moi_constraints.md docs/DOCUMENTATION.mdjulia +release --project=docs/ docs/make.jl localjulia +release --project=. -e 'using Pkg; Pkg.test()'git diff --checkAll checks pass locally with Julia 1.12.0.
Branch Hygiene
mainat2a7a6000e02e75923280f68b30bd7debcb5a85d7with append-only merge commit6ddd51f.pr-10-jump-moi-designat988bef5ee04e97fd9a9c7620dd47c93ca5096aac.docs/DOCUMENTATION.mdin the adjacent internal-note inventory. The changes are semantically disjoint, but this branch should be refreshed again if Add PEPS docs, examples, and benchmarks #46 lands first.