Add Wishart synthesis metadata schema - #111
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Review as a repository maintainer. Note: I am the PR author, so GitHub only permits a COMMENT review here; the formal verdict (approval or change request) must come from another maintainer. There are no blocking issues; the items below are nonblocking and questions.
Intent vs issue #110: the PR delivers the three acceptance criteria. Wishart-generated models emit schema-backed synthesis metadata; the schema and docs describe the model/parameters fields and types; and the negative test catches an incompatible shape. Closes #110 is appropriate.
Schema design is sound: the oneOf between wishart_synthesis_metadata (model enum Wishart) and generic_synthesis_metadata (model not enum Wishart) is mutually exclusive on model, so exactly one branch matches valid data and a malformed Wishart block (e.g. missing n) fails both branches and is rejected as intended.
Blocking
None.
Nonblocking
-
No test exercises the new
generic_synthesis_metadatabranch. That definition is the mechanism that keeps non-Wishart synthesis metadata (e.g. Sherrington-Kirkpatrick, which emitsmodel = "Sherrington-Kirkpatrick"withparameters {n, mu, sigma}insrc/library/synthesis/sherrington_kirkpatrick.jl) valid after this schema tightening. I verified manually that an SK model round-trips throughwrite_model/read_modeltoday, but there is no regression test, so a future change to the generic branch could silently start rejecting SK output. Add a round-trip/validation test for a non-Wishart synthesis model. -
Backwards-compatibility tightening. Before this PR
metadata.synthesiswas unconstrained, soread_modelaccepted any value there. Now every file with asynthesiskey must match one of the two definitions orread_modelthrowsFormatError. Package-generated files (Wishart, SK) conform, but hand-authored or third-party bqpjson files using a differentsynthesisshape will newly fail to read. Worth a CHANGELOG/release note flagging the stricter validation. -
Doc gap.
docs/src/formats/bqpjson.mddocuments only the Wishart shape. The generic contract that is now enforced for all other generators (modelany string other thanWishart, plus aparametersobject) is undocumented even though it now gates reads.
Questions
-
The negative test asserts only
@test_throws QUBOTools.FormatError, not that the failure originates in the synthesis block. The crafted JSON is otherwise valid, so it currently fails for the right reason, but a future unrelated schema change to that fixture could make it pass for the wrong reason. Consider asserting on the error message referencingsynthesis. -
The
generatepath being schematized is deprecated (it warns and points users to QUBOLib.jl, seesrc/library/synthesis/abstract.jl). Is schema-backing this metadata here still the intended home, or should the canonical synthesis-metadata contract live in QUBOLib? Issue #110 says it is worth resolving regardless, so this is a direction check, not an objection.
Tests run
jq empty src/library/format/bqpjson/bqpjson.schema.json- passed (valid JSON).julia +1.12 --project=. -e 'using Pkg; Pkg.test()'- passed, 1152/1152 (matches the PR description). New Wishart testsets run under Formats/BQPJSON.- Manual check: SK-generated model written and re-read through bqpjson schema validation - round-trips cleanly, confirming the generic branch works.
Merge readiness
Mergeable, no blocking issues. The change is correct and tested for the Wishart case. I recommend adding the generic-branch test (item 1) and a release note (item 2) before or alongside merge, but neither blocks. Because I am the PR author this is submitted as COMMENT; a second maintainer should provide the formal approval.
| "dwig_generator": { | ||
| "type": "string" | ||
| }, | ||
| "synthesis": { |
There was a problem hiding this comment.
Backwards-compatibility note: previously metadata.synthesis was unconstrained and read_model accepted any value here. With this oneOf, every file carrying a synthesis key must now match one of the two definitions or read_model throws FormatError. Package-generated files conform, but hand-authored/third-party files using a different synthesis shape will newly fail to read. Worth a CHANGELOG/release note. Nonblocking.
There was a problem hiding this comment.
Addressed in 548379c. I added an Unreleased changelog note calling out the stricter BQPJSON metadata.synthesis validation in CHANGELOG.md.
| } | ||
| }, | ||
| "definitions": { | ||
| "generic_synthesis_metadata": { |
There was a problem hiding this comment.
This generic branch is the only thing keeping non-Wishart synthesis metadata valid after the tightening (e.g. Sherrington-Kirkpatrick emits model = "Sherrington-Kirkpatrick" with parameters {n, mu, sigma}), but no test exercises it. I verified an SK model round-trips through write_model/read_model today; please add a regression test so a future edit here cannot silently break non-Wishart reads. Nonblocking.
There was a problem hiding this comment.
Addressed in 548379c. I added a Sherrington-Kirkpatrick BQPJSON write/read regression test in test/unit/library/formats.jl to cover the generic synthesis metadata branch.
|
|
||
| ``` | ||
|
|
||
| ## Synthesis Metadata |
There was a problem hiding this comment.
Only the Wishart shape is documented. The generic contract now enforced for every other generator (model = any string other than Wishart, plus a parameters object) gates reads but is undocumented. Consider adding a short note. Nonblocking.
There was a problem hiding this comment.
Addressed in 548379c. The BQPJSON docs now describe the generic non-Wishart metadata.synthesis contract and keep the Wishart-specific shape documented separately.
| } | ||
| """) | ||
|
|
||
| @test_throws QUBOTools.FormatError QUBOTools.read_model(temp_path) |
There was a problem hiding this comment.
This asserts only the error type, not that the failure comes from the synthesis block. The fixture is otherwise valid so it currently fails for the right reason, but an unrelated future change to this JSON could make it pass for the wrong reason. Consider asserting the error message references synthesis. Question/nit.
There was a problem hiding this comment.
Addressed in 548379c. The negative Wishart test now catches the FormatError and asserts the rendered error includes [metadata][synthesis].
|
Review comments addressed in Commits pushed:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
Performance Report -
|
| case | main |
dev |
diff |
|---|---|---|---|
/constructors/n=128/Model/MOI/bool |
190.666 μs (209.521 μs) ± 236.692 μs | 195.706 μs (219.865 μs) ± 270.936 μs | invariant (2.64%) |
/constructors/n=128/Model |
656.193 μs (692.491 μs) ± 706.552 μs | 657.438 μs (688.546 μs) ± 798.870 μs | invariant (0.19%) |
/constructors/n=2048/Model/MOI/bool |
4.888 ms (5.760 ms) ± 19.487 ms | 4.979 ms (5.860 ms) ± 19.648 ms | invariant (1.86%) |
/constructors/n=2048/Model |
27.291 ms (29.533 ms) ± 19.973 ms | 27.319 ms (29.759 ms) ± 19.853 ms | invariant (0.10%) |
/constructors/n=384/Model/MOI/bool |
539.526 μs (591.172 μs) ± 2.978 ms | 562.771 μs (639.714 μs) ± 3.159 ms | invariant (4.31%) |
/constructors/n=384/Model |
2.693 ms (2.791 ms) ± 1.093 ms | 2.647 ms (2.826 ms) ± 1.224 ms | invariant (-1.71%) |
/constructors/tsp/cities=36/Model/MOI/bool |
17.466 ms (18.904 ms) ± 48.438 ms | 17.313 ms (19.405 ms) ± 43.532 ms | invariant (-0.88%) |
/constructors/tsp/cities=36/Model |
60.417 ms (63.199 ms) ± 52.514 ms | 60.590 ms (64.051 ms) ± 44.255 ms | invariant (0.29%) |
/conversions/n=128/form/Dict |
140.913 μs (171.685 μs) ± 2.470 ms | 150.561 μs (186.107 μs) ± 2.555 ms | invariant (6.85%) |
/conversions/n=128/form/Matrix/Float32 |
37.039 μs (45.199 μs) ± 120.017 μs | 39.214 μs (50.665 μs) ± 174.279 μs | invariant (5.87%) |
/conversions/n=128/form/Matrix |
51.706 μs (68.949 μs) ± 2.472 ms | 52.990 μs (78.547 μs) ± 2.576 ms | invariant (2.48%) |
/conversions/n=128/form/SparseMatrixCSC |
99.045 μs (112.835 μs) ± 154.554 μs | 105.086 μs (119.022 μs) ± 227.877 μs | invariant (6.10%) |
/conversions/n=128/ising/Matrix |
52.527 μs (68.382 μs) ± 2.484 ms | 55.534 μs (77.129 μs) ± 2.576 ms | invariant (5.72%) |
/conversions/n=128/qubo/Matrix/min |
633.551 μs (734.769 μs) ± 7.117 ms | 655.013 μs (748.172 μs) ± 6.497 ms | invariant (3.39%) |
/conversions/n=128/qubo/Matrix |
331.017 μs (385.523 μs) ± 5.311 ms | 354.111 μs (398.775 μs) ± 4.889 ms | invariant (6.98%) |
/conversions/n=384/form/Dict |
337.639 μs (510.392 μs) ± 4.097 ms | 319.447 μs (451.577 μs) ± 4.054 ms | invariant (-5.39%) |
/conversions/n=384/form/Matrix/Float32 |
204.180 μs (349.502 μs) ± 10.574 ms | 220.231 μs (371.183 μs) ± 9.978 ms | invariant (7.86%) |
/conversions/n=384/form/Matrix |
395.578 μs (657.620 μs) ± 17.247 ms | 389.107 μs (700.217 μs) ± 16.107 ms | invariant (-1.64%) |
/conversions/n=384/form/SparseMatrixCSC |
295.531 μs (373.432 μs) ± 8.784 ms | 332.290 μs (386.943 μs) ± 8.597 ms | invariant (12.44%) |
/conversions/n=384/ising/Matrix |
408.321 μs (668.686 μs) ± 17.515 ms | 408.321 μs (708.170 μs) ± 16.413 ms | invariant (0.00%) |
/conversions/n=384/qubo/Matrix/min |
6.397 ms (6.891 ms) ± 33.198 ms | 6.425 ms (6.935 ms) ± 33.112 ms | invariant (0.43%) |
/conversions/n=384/qubo/Matrix |
3.243 ms (3.795 ms) ± 27.234 ms | 3.242 ms (3.872 ms) ± 24.062 ms | invariant (-0.03%) |
/evaluation/n=128/value/dense-form |
346.365 μs (373.440 μs) ± 600.609 μs | 345.344 μs (370.531 μs) ± 485.734 μs | invariant (-0.29%) |
/evaluation/n=128/value/dict-form |
242.462 μs (245.898 μs) ± 7.688 μs | 239.527 μs (242.331 μs) ± 7.508 μs | invariant (-1.21%) |
/evaluation/n=128/value/model |
49.472 μs (76.883 μs) ± 615.746 μs | 49.261 μs (50.845 μs) ± 353.971 μs | invariant (-0.43%) |
/evaluation/n=128/value/sparse-form |
49.071 μs (75.541 μs) ± 611.375 μs | 48.871 μs (51.641 μs) ± 425.821 μs | invariant (-0.41%) |
/evaluation/n=384/value/dense-form |
4.108 ms (4.137 ms) ± 124.066 μs | 4.118 ms (4.150 ms) ± 141.470 μs | invariant (0.24%) |
/evaluation/n=384/value/dict-form |
481.829 μs (495.814 μs) ± 20.282 μs | 477.129 μs (489.693 μs) ± 11.545 μs | invariant (-0.98%) |
/evaluation/n=384/value/model |
163.635 μs (168.834 μs) ± 116.974 μs | 161.411 μs (170.067 μs) ± 120.624 μs | invariant (-1.36%) |
/evaluation/n=384/value/sparse-form |
162.763 μs (169.235 μs) ± 125.242 μs | 164.186 μs (169.506 μs) ± 134.050 μs | invariant (0.87%) |
Summary
metadata.synthesis.model = "Wishart"with integerparameters.nandparameters.m, while preserving generic synthesis metadata for other generators.Tests
jq empty src/library/format/bqpjson/bqpjson.schema.json- passed.julia +1.12 --project=. -e 'using Pkg; Pkg.test()'- passed, 1152 tests.Branch Hygiene
main.origin/mainatbafccc7310f4a9cea57d9eedfdf890a6d498fd60.main.Closes #110