Add optional GenericTensorNetworks backend#31
Conversation
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
GitHub would not allow REQUEST_CHANGES because the authenticated account owns this PR, but the findings below are blocking and should be treated as request-changes.
Review summary:
- Blocking issues:
GTNBackend.k > 1paths exposed by the public API are broken for Float64 QUBOs.:kbest_sizesfails when combining GTN/Tropical sizes with the objective constant, and:configs/:countfail before TenSolver can return a result. These need either implementation plus tests or explicit rejection of unsupported combinations. - Nonblocking issues: the one-variable DMRG path silently drops unknown kwargs, and
solution_spacegives an opaque failure when used with DMRG. - Questions: none.
- Tests run and outcomes:
julia +1.12 --project=. -e 'using Pkg; Pkg.test(; coverage=false)'passed;julia +1.12 --project=docs/ docs/make.jlpassed; targetedGTNBackend(k=2)smoke tests failed as noted inline. - Merge recommendation: I would not merge this until the blocking issues above are addressed.
| item = _scalar(raw) | ||
|
|
||
| size = _try_read_size(item) | ||
| objective = isnothing(size) ? model.constant : model.constant + _primary_size(size) |
There was a problem hiding this comment.
Blocking: The k > 1 size path currently returns GTN/Tropical values, so adding model.constant fails for a supported public option. Repro:
using TenSolver, GenericTensorNetworks, ProblemReductions
Q = [0.0 2.0; 0.0 0.0]
l = [-1.0, -1.0]
TenSolver.solution_space(Q, l, 3.0; backend=TenSolver.GTNBackend(property=:kbest_sizes, k=2))This throws promotion of types Float64 and Tropical{Float64} failed at this line. Please unwrap GTN/Tropical size values to the underlying numeric objective before combining with the constant, or reject this property/type combination with a clear ArgumentError, and add a regression test for k > 1.
There was a problem hiding this comment.
Addressed in c96eb40: GTN/Tropical numeric wrappers are now unwrapped before objective arithmetic, and GTNBackend(property=:kbest_sizes, k=2) is covered in test/gtn.jl.
| elseif property in (:count, :degeneracy) | ||
| return k == 1 ? GenericTensorNetworks.CountingMin() : GenericTensorNetworks.CountingMin(k) | ||
| elseif property in (:configs, :enumerate) | ||
| return k == 1 ? GenericTensorNetworks.ConfigsMin(; bounded=backend.bounded, tree_storage=backend.tree_storage) : |
There was a problem hiding this comment.
Blocking: These ConfigsMin(k)/CountingMin(k) branches are exposed by GTNBackend.k, but they fail on a simple Float64 QUBO before TenSolver returns a result. GTNBackend(property=:configs, k=2) throws promotion of types Max2Poly{ConfigEnumerator...} and Float64 failed; :count, k=2 fails similarly. This makes the public k API only partially functional and untested. Please either implement a representation/typed conversion that GenericTensorNetworks supports for these k-best properties, or reject unsupported k/property/coefficient-type combinations up front, and cover the accepted behavior in test/gtn.jl.
There was a problem hiding this comment.
Addressed in c96eb40: unsupported :configs/:count with k > 1 now fail up front with ArgumentError, while :single, k > 1 remains supported and tested in test/gtn.jl.
| verbosity = 1, | ||
| on_iteration :: Union{Nothing, Function} = nothing, | ||
| callback_every :: Int = 1, | ||
| kwargs... |
There was a problem hiding this comment.
Nonblocking: The exact one-variable path accepts arbitrary kwargs... and drops them, while the multi-variable DMRG path rejects unknown solver keywords through _minimize's signature. That means a typo such as iteration=5 is silently ignored only for one-variable models. Consider either listing the DMRG keywords that are intentionally ignored here or throwing on non-empty unexpected kwargs after removing the accepted solver knobs.
There was a problem hiding this comment.
Addressed in c96eb40: the one-variable path now accepts the known DMRG kwargs for compatibility and throws ArgumentError for unexpected keywords. Covered in test/backends.jl.
| model = pseudoboolean(args...) | ||
| selected_backend = backend isa AbstractBackend ? backend : backend_from_attribute(backend; property=isnothing(property) ? :configs : property) | ||
| selected_property = isnothing(property) && selected_backend isa GTNBackend ? selected_backend.property : (isnothing(property) ? :configs : property) | ||
| return _solve_backend(selected_backend, model; property=selected_property, kwargs...) |
There was a problem hiding this comment.
Nonblocking: solution_space(...; backend=:dmrg) reaches this call with property=:configs, then fails later as an unsupported keyword to _minimize. Since solution_space is a new public API, it would be better to reject backends without solution-space support here with a direct ArgumentError.
There was a problem hiding this comment.
Addressed in c96eb40: solution_space(...; backend=:dmrg) now raises a direct ArgumentError for unsupported solution-space backends. Covered in test/backends.jl.
|
Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up items:
|
bernalde
changed the title
1 participant
Summary
DMRGBackendand optionalGTNBackendwithout making GenericTensorNetworks a hard dependency.ProblemReductions.QUBOand higher-order PUBO terms to a small custom CSP.GTNSolution,solution_space, QUBODrivers backend attributes, tests, and docs/README examples.Notes
Validation
julia +1.12 --project=. -e "using Pkg; Pkg.resolve(); Pkg.instantiate()"julia +1.12 --project=. -e "using Pkg; Pkg.test(; coverage=false)"