docs: state the stability policy, and take the free complexity win - #23
Merged
Conversation
Closes the two remaining quality findings. #21 -- the API has been stable since 0.1.x and the README promises a drop-in replacement, but 0.x carries no semver obligation, so a reader could not tell whether that stability was a commitment or an accident. A Stability section now splits it explicitly: the solve_qp signature, the Solution field order and the two verbatim ValueError strings are covered; the underscored modules, the internal Q/R sign conventions, whether a problem takes the unit fast path, and bit-exact results are not. Shipping 1.0.0 remains the alternative and is a release decision, not a documentation one. #22 -- measured where the complexity actually lives by deleting each candidate from a scratch copy and re-running radon. The inner loop carries 15 of the 23: stubbing it out drops solve_qp to A (4), which makes it the only route to a B and also the one place indirection is not free, since it is the hot loop. The argument defaulting was the opposite case -- separable, runs once per call, no hot-path cost -- so it moves to _default_constraints, taking solve_qp from D (23) to C (19) and the package average from 4.47 to 4.25. Verified the extraction is free rather than assuming it. There is no benchmarks folder, so `make benchmark` is a no-op; timed directly instead, best-of-five on box-constrained problems: n=10 0.0236 -> 0.0234 ms, n=25 0.0290 -> 0.0288, n=50 0.0631 -> 0.0629. Within noise in the direction of faster. Also corrects the layout table, which still described _qr.py as "Givens QR insert/delete" after insertion moved to a Householder reflection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR documents the project’s pre-1.0 stability commitments for the public solve_qp surface (closing #21), and reduces solve_qp’s measured cyclomatic complexity by extracting argument defaulting into a helper without impacting runtime behavior (partial progress on #22). It also corrects the README’s layout description for _qr.py.
Changes:
- Add a README Stability section that explicitly states what API/behavior is and isn’t covered by compatibility guarantees while
0.x. - Extract
solve_qp’sC/bdefaulting and coercion into_default_constraints, reducingsolve_qp’s complexity from D (23) to C (19). - Update the README layout table to reflect Householder insertion + Givens deletion in
_qr.py.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/cvx/quadprog/_solve.py |
Refactors constraint defaulting into _default_constraints and updates the recorded complexity rationale comment. |
README.md |
Adds a Stability policy section and fixes the _qr.py layout description. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #21. Addresses #22 — see the note at the end on how that one should
close.
#21 — a stated stability policy
The surface has been stable since 0.1.x and the README promises a drop-in
replacement, but
0.xcarries no semver obligation, so a reader could not tellwhether that stability was a commitment or an accident.
A Stability section now says which it is:
solve_qpsignature — names, order, defaults_solve/_qrreached directlySolutionfield names and order, so 6-way unpacking keeps workingQ/Rsign conventionsValueErrorstrings reproduced verbatim from the referenceCcolumn-wise,>=)Graises or propagates NaNsShipping 1.0.0 was the other option the issue offered. That is a release
decision rather than a documentation one, so it is left to you — the section
closes the finding either way, and would need only a wording change if 1.0
follows.
#22 — measured, then took the free half
The complexity was measured rather than argued about, by deleting each
candidate from a scratch copy and re-running radon:
solve_qpThat third row is the finding: the inner loop carries 15 of the 23. It is
therefore the only route to a B — and it is also the hot loop, so lifting it out
means threading
xv, uv, obj, iact, nact, J, R, u, slack, iter_partialin andreturning five, or hiding them behind an object whose attribute lookups land in
the innermost iteration. The README's own benchmarks show per-iteration dispatch
dominating below
n ≈ 160, which is exactly where this package is alreadyslower than the C reference.
The argument defaulting was the opposite case — separable, runs once per call,
no hot-path cost. It moves to
_default_constraints, which takessolve_qpfrom D (23) to C (19) and the package average from 4.47 to 4.25.
The "free at runtime" claim was verified, not assumed. There is no
benchmarks/folder, somake benchmarkis a no-op — timed directly instead,best-of-five on box-constrained problems:
Within noise, in the direction of faster. (Not comparable to the README's table
— different problem setup — but both columns ran the same script minutes apart.)
How #22 should close
solve_qpis C (19), not B, so the issue's first acceptance criterion is notmet and this PR does not auto-close it. Its second criterion is
wontfixwith the decision recorded, which is now the case: the commentabove
solve_qpcarries the measurement, the reason the inner loop is the wrongthing to extract, and the trigger for revisiting (the function gaining a new
responsibility — a second pivoting rule, an alternative factorisation).
Also
Corrects the layout table, which still described
_qr.pyas "Givens QRinsert/delete" after insertion moved to a Householder reflection.
Verification
make fmtmake typecheckty+mypy --strictmake testmake rhiza-testcheck_test_layout.pyradon cc src -aThe extraction is behaviour-preserving: same 982 tests, still 100% branch
coverage, including the
C-without-bandb-without-Cerror paths thatmoved into
_default_constraints.