Implement variable fixing for forms - #102
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues:
- Blocking:
lift_stateaccepts a reduced state that is longer than the index map and silently drops the extra entries. That can let callers evaluate or store a lifted solution that was not the reduced solution they passed in. I reproduced this on the PR head withQUBOTools.lift_state([0, 1], Dict{Int,Int}(), Dict(1 => 1), 1), which returns[0]instead of throwing. Require the reduced state length to match the number of surviving variables, and add a regression test for the too-long case.
Nonblocking issues: none.
Questions: none.
Tests run and outcomes:
- Confirmed GitHub and local diff totals match: 7 files changed, 314 insertions, 9 deletions.
- Read linked issue #99 and verified this PR targets the requested
fix_variablesimplementation and state lifting API. julia +1.12 --project=. -e 'using Test, Random, SparseArrays, QUBOTools; include("test/assets/comparison.jl"); include("test/unit/library/form.jl"); test_form_fix_variables()': passed, 390 tests.julia +1.12 --project=. -e 'using Pkg; Pkg.test()': passed, 1105 tests. The local test environment re-resolved in/tmpbecause the test manifest differs from the root manifest.- Manual reproduction:
QUBOTools.lift_state([0, 1], Dict{Int,Int}(), Dict(1 => 1), 1)returned[0].
Merge-readiness: I would not merge this until the blocking issue above is addressed. Because I am posting from the PR author's GitHub account, GitHub requires this review to be submitted as COMMENT; the formal approval or change request must come from another maintainer.
| push!(seen, j) | ||
| end | ||
|
|
||
| length(fixed) + length(index_map) == n || |
There was a problem hiding this comment.
Blocking: this validation only checks that mapped indices are in bounds and non-duplicated; it does not check that every entry of state_reduced is actually used. For example, lift_state([0, 1], Dict{Int,Int}(), Dict(1 => 1), 1) returns [0] and silently ignores the extra reduced value. Please require length(index_map) == length(state_reduced) or otherwise verify that seen covers 1:m, and add a regression test for too-long reduced states.
There was a problem hiding this comment.
Addressed in 4b165da. _lift_state_validate now requires the reduced state length to match index_map, and test_form_fix_variables() covers the too-long reduced-state case. Focused tests now pass with 396 checks; full Pkg.test() passes with 1111 checks.
|
Addressed the unresolved review thread. Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
Performance Report -
|
| case | main |
dev |
diff |
|---|---|---|---|
/constructors/n=128/Model/MOI/bool |
578.871 μs (621.310 μs) ± 333.285 μs | 595.521 μs (643.892 μs) ± 426.140 μs | invariant (2.88%) |
/constructors/n=128/Model |
659.462 μs (691.737 μs) ± 665.328 μs | 670.672 μs (698.744 μs) ± 726.702 μs | invariant (1.70%) |
/constructors/n=2048/Model/MOI/bool |
25.151 ms (26.153 ms) ± 35.513 ms | 25.310 ms (26.676 ms) ± 35.452 ms | invariant (0.63%) |
/constructors/n=2048/Model |
27.258 ms (29.877 ms) ± 19.219 ms | 27.416 ms (30.635 ms) ± 2.014 ms | invariant (0.58%) |
/constructors/n=384/Model/MOI/bool |
2.266 ms (2.397 ms) ± 5.740 ms | 2.345 ms (2.462 ms) ± 523.724 μs | invariant (3.49%) |
/constructors/n=384/Model |
2.679 ms (2.792 ms) ± 1.110 ms | 2.715 ms (2.835 ms) ± 1.254 ms | invariant (1.35%) |
/conversions/n=128/form/Dict |
140.913 μs (182.587 μs) ± 2.514 ms | 158.967 μs (194.232 μs) ± 2.609 ms | invariant (12.81%) |
/conversions/n=128/form/Matrix/Float32 |
36.298 μs (44.864 μs) ± 112.090 μs | 43.291 μs (55.053 μs) ± 184.693 μs | regression (19.27%) |
/conversions/n=128/form/Matrix |
58.239 μs (73.162 μs) ± 2.503 ms | 62.046 μs (78.447 μs) ± 2.519 ms | invariant (6.54%) |
/conversions/n=128/form/SparseMatrixCSC |
159.538 μs (172.072 μs) ± 157.879 μs | 171.149 μs (192.590 μs) ± 231.616 μs | invariant (7.28%) |
/conversions/n=128/ising/Matrix |
53.881 μs (70.582 μs) ± 2.460 ms | 61.163 μs (78.271 μs) ± 2.614 ms | invariant (13.51%) |
/conversions/n=128/qubo/Matrix/min |
643.562 μs (745.944 μs) ± 7.079 ms | 662.397 μs (769.286 μs) ± 6.653 ms | invariant (2.93%) |
/conversions/n=128/qubo/Matrix |
356.065 μs (400.092 μs) ± 5.267 ms | 367.506 μs (422.349 μs) ± 5.065 ms | invariant (3.21%) |
/conversions/n=384/form/Dict |
296.674 μs (522.771 μs) ± 3.837 ms | 322.912 μs (464.647 μs) ± 3.881 ms | invariant (8.84%) |
/conversions/n=384/form/Matrix/Float32 |
205.945 μs (341.147 μs) ± 10.343 ms | 225.191 μs (361.074 μs) ± 9.548 ms | invariant (9.35%) |
/conversions/n=384/form/Matrix |
378.908 μs (651.491 μs) ± 17.169 ms | 394.637 μs (665.477 μs) ± 16.084 ms | invariant (4.15%) |
/conversions/n=384/form/SparseMatrixCSC |
1.177 ms (1.474 ms) ± 7.109 ms | 1.222 ms (1.532 ms) ± 7.152 ms | invariant (3.90%) |
/conversions/n=384/ising/Matrix |
379.318 μs (644.885 μs) ± 16.595 ms | 405.136 μs (666.293 μs) ± 16.263 ms | invariant (6.81%) |
/conversions/n=384/qubo/Matrix/min |
6.523 ms (6.806 ms) ± 32.363 ms | 6.596 ms (6.820 ms) ± 33.551 ms | invariant (1.12%) |
/conversions/n=384/qubo/Matrix |
3.157 ms (3.775 ms) ± 26.364 ms | 3.256 ms (3.794 ms) ± 25.534 ms | invariant (3.14%) |
/evaluation/n=128/value/dense-form |
347.529 μs (375.121 μs) ± 587.230 μs | 345.535 μs (370.922 μs) ± 502.502 μs | invariant (-0.57%) |
/evaluation/n=128/value/dict-form |
255.567 μs (256.930 μs) ± 7.402 μs | 236.011 μs (239.497 μs) ± 6.811 μs | invariant (-7.65%) |
/evaluation/n=128/value/model |
49.292 μs (76.473 μs) ± 618.830 μs | 49.252 μs (51.226 μs) ± 369.800 μs | invariant (-0.08%) |
/evaluation/n=128/value/sparse-form |
49.042 μs (76.433 μs) ± 694.701 μs | 49.021 μs (53.364 μs) ± 444.691 μs | invariant (-0.04%) |
/evaluation/n=384/value/dense-form |
4.080 ms (4.107 ms) ± 138.631 μs | 4.100 ms (4.129 ms) ± 137.420 μs | invariant (0.49%) |
/evaluation/n=384/value/dict-form |
457.735 μs (466.236 μs) ± 33.069 μs | 471.410 μs (480.272 μs) ± 12.024 μs | invariant (2.99%) |
/evaluation/n=384/value/model |
162.324 μs (167.242 μs) ± 112.314 μs | 166.551 μs (172.452 μs) ± 138.253 μs | invariant (2.60%) |
/evaluation/n=384/value/sparse-form |
162.714 μs (167.423 μs) ± 113.722 μs | 166.200 μs (171.951 μs) ± 153.842 μs | invariant (2.14%) |
Summary
fix_variables(form, fix::AbstractDict)forAbstractFormbackends by folding fixed linear and quadratic contributions into the reduced form offset and surviving linear terms.lift_stateto reconstruct full states from reduced states, fixed values, and the returned old-to-new index map.Closes #99
Tests
julia +1.12 --project=. -e 'using Test, Random, SparseArrays, QUBOTools; include("test/assets/comparison.jl"); include("test/unit/library/form.jl"); test_form_fix_variables()'passed: 390 variable-fixing tests.julia +1.12 --project=. -e 'using Pkg; Pkg.test()'passed: 1105 tests.Local note: the default
julialauncher is Julia 1.10, but the checked-in manifest is resolved with Julia 1.12; local validation used Julia 1.12. CI still covers the configured Julia 1.10 and latest-stable matrix.Downstream Search
gh search code fix_variables --repo JuliaQUBO/ToQUBO.jl --json path,repository,url --limit 100: no references.gh search code fix_variables --repo JuliaQUBO/QUBODrivers.jl --json path,repository,url --limit 100: no references.gh search code fix_variables --repo JuliaQUBO/QUBOLib.jl --json path,repository,url --limit 100: no references.Branch Hygiene
mainorigin/mainata3637a0