docs: pin the Solution tuple ordering with a worked doctest - #100
Merged
Conversation
Add a hand-checkable example to the `Solution` docstring: identity `G` makes the objective separable, so `x >= 0` clips the negative entries of `a` and every reported field can be read off by hand -- including the two that surprise, `iact` being 1-based and `iterations` counting outer iterations rather than active constraints. Also record why `_solve_with_factors` stays one function: it is the outer loop of Goldfarb & Idnani (1983) transcribed in the paper's order, sharing working state that splitting would thread through single-call-site helpers. The comment names the measurements and the signal to revisit -- a radon rank change, not the decimals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves documentation clarity by adding a concrete, hand-checkable example to the Solution docstring that pins down the tuple iteration/unpacking order and clarifies two commonly surprising fields (iact 1-based indexing and iterations semantics). It also adds a measured rationale comment explaining why _solve_with_factors intentionally remains a single function despite being the highest-complexity block in the package.
Changes:
- Add a worked doctest-style example to
Solution’s docstring demonstrating unpack order and expected field values on a simpleG = I,x >= 0problem. - Document (via comment) the measured complexity/maintainability context for keeping
_solve_with_factorsas one function.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/cvx/quadprog/_base.py | Adds a worked example in the Solution docstring to pin tuple ordering and explain iact/iterations. |
| src/cvx/quadprog/_solve.py | Adds a rationale comment documenting why _solve_with_factors stays monolithic, referencing measured radon metrics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Documentation only — no behaviour, no numerics, no performance claims touched.
Solutiondocstring: a worked exampleThe docstring promised that iteration order matches
quadprog.solve_qp's six-tuple but never showed it. The new doctest picks a problem small enough to verify by hand: withG = Ithe objective separates intox_i²/2 − a_i x_iper coordinate, so underx ≥ 0the minimiser isawith negative entries clipped to zero.It exists mainly to pin the two fields that surprise people:
iactis 1-based — only the second constraint binds, so it reads[2].iterationscounts outer iterations, not active constraints, so[2, 0]here exceeds the single binding constraint: one iteration broughtx₂onto its bound, a final one confirmed nothing was left to add.Verified with
python -m doctest src/cvx/quadprog/_base.py— 10 tests, all passing. Note that doctests are not collected by the suite (pytest.inisets no--doctest-modules), so this is a readable example rather than a new gate._solve_with_factors: why it stays one functionradon ccputs this block at B (10) — the worst in the package, against an A (3.96) average over 54 blocks with nothing at C or worse — and_solve.pyhas the lowest MI of the ten modules, mid-20s of band A. The comment records that this is measured and deliberate rather than unexamined: the function transcribes the outer loop of Goldfarb & Idnani (1983), whose steps share working state (J, packedR,nact, the active index vector) and read in the paper's order. Splitting it would thread that state through helpers called once each, in sequence, from one site — trading a legible transcription for a less legible one and weakening the correspondence to the paper this file is organised around. The per-iteration work already lives in_steps.pyand_qr.py.The MI figure is quoted as a band, not a decimal, on purpose: MI folds in Halstead volume, so the comment lowers the number it reports. CC is comment-invariant; MI is not. The stated signal to revisit is a rank change — this block reaching C, or the module dropping out of MI band A — not drift in the decimals.
🤖 Generated with Claude Code