Fix missing array dimensions in QUBin sparse constructor calls - #58
Conversation
|
@copilot Address this: |
Added |
There was a problem hiding this comment.
Pull request overview
This PR fixes QUBin parsing so sparse linear/quadratic forms preserve the declared model dimension n even when some variables have no terms (e.g., trailing variables).
Changes:
- Pass
nintosparsevec(li, lv, n)when parsing the linear form. - Pass
(n, n)intosparse(qi, qj, qv, n, n)when parsing the quadratic form. - Add a unit test covering a 3-variable model where variable 3 has no linear/quadratic terms, and enable Julia CI caching.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/library/format/qubin/parser.jl |
Ensures parsed sparse vector/matrix are constructed with the correct explicit dimensions. |
test/unit/library/formats.jl |
Adds a regression test intended to validate dimension preservation for sparse forms with “missing” variables. |
.github/workflows/ci.yml |
Adds julia-actions/cache@v2 to speed up CI package operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
bernalde
left a comment
There was a problem hiding this comment.
I reproduced the regression on main and confirmed the parser change fixes it. The remaining issue is test coverage: the new QUBin regression test still passes on main, so it does not actually guard the broken behavior.
Tested locally on this branch with Julia 1.12.0:
JULIA_DEPOT_PATH=$PWD/.julia-depot:$HOME/.julia julia --project=. -e "using Pkg; Pkg.test()"JULIA_DEPOT_PATH=$PWD/.julia-depot:$HOME/.julia QUBOTOOLS_FOREIGN_TESTS=true julia --project=. -e "using Pkg; Pkg.test()"
Both passed here. On main, a QUBin round-trip for a model with an unreferenced variable still fails at QUBOTools.value(dst_model, [1, 0, 1]) with DimensionMismatch: Vector x has a length 1 but y has a length 3.
Updated in commit 621176d. The test now calls |
bernalde
left a comment
There was a problem hiding this comment.
I reproduced the bug on main and confirmed the parser fix is correct. The value() assertions in the latest commit are the right approach to catch the regression, since dimension() reads the stored n from the HDF5 file (the Form.n field) rather than deriving it from the sparse array sizes, and _compare_models only compares non-zero terms via findnz, meaning neither check would detect undersized sparse structures.
Testing summary (Julia 1.12.0, Linux):
- All 612 tests pass on the PR branch (including the new "sparse dimensions" testset).
- On
main,value(dst_model, [1, 0, 1])throwsDimensionMismatch("Vector x has a length 1 but y has a length 3")exactly as expected. - The expected values in the assertions are correct: for the model with
L = {1 => 1.0},Q = {(1,2) => 2.0},value([1,0,1]) = 1.0andvalue([1,1,1]) = 3.0.
The core fix (passing n to sparsevec and sparse) is minimal and correct. There are a few items to address before merging, noted inline.
Co-authored-by: bernalde <8647329+bernalde@users.noreply.github.com>
Co-authored-by: bernalde <8647329+bernalde@users.noreply.github.com>
…to .gitignore Co-authored-by: bernalde <8647329+bernalde@users.noreply.github.com>
…ated .gitignore entry Co-authored-by: bernalde <8647329+bernalde@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
31a4cba to
a8103cb
Compare
The QUBin parser used
sparse(I, J, V)andsparsevec(I, V)without explicit dimensions, causing Julia to infer size from max indices. This produces wrong dimensions when some variables have no linear/quadratic terms, leading toDimensionMismatcherrors when callingQUBOTools.value(model, state)on the deserialized model.Changes
src/library/format/qubin/parser.jl: Pass dimensionn(already read from file) to sparse constructorssparsevec(li, lv)→sparsevec(li, lv, n)sparse(qi, qj, qv)→sparse(qi, qj, qv, n, n)test/unit/library/formats.jl: Added regression test for a 3-variable model where variable 3 has no terms. The test performs a QUBin write/read round-trip and verifies:length(L) == 3,size(Q) == (3, 3)) via internal form accessors, providing a clear failure message if the bug resurfacesQUBOTools.value(dst_model, [1, 0, 1]) == 1.0andQUBOTools.value(dst_model, [1, 1, 1]) == 3.0, which exerciseL' * ψandψ' * Q * ψand would throwDimensionMismatchon the unfixed parser.github/workflows/ci.yml: Addedjulia-actions/cache@v2to cache the Julia depot and speed up CI runs.Example
Original prompt
sparse(I, J, V, [m, n])constructor #55💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.