refactor(mprgp): extract the three moves, and document the solvers by example - #88
Merged
Merged
Conversation
… example MPRGP's loop body held the conjugate-gradient, expansion and proportioning moves inline, so the proportioning switch and the stopping test were buried in the numerics of the moves themselves. Each move is now its own function over an `Iterate = (x, g, p)` state, with `_proportional_step` making the feasibility choice between the CG and expansion moves and reporting the Hessian products it consumed. `_mprgp` keeps only the loop, the stopping test and the switch; the per-move counters become a dict keyed by the move name the step returns. Add doctests to `ActiveSetSolver`, `MPRGP` and the `nncg.inner` module: the operator-not-array calling convention, the KKT certificate scoring the answer independently, the interchangeable inner solvers, and MPRGP's step counts summing to `iterations`. Behaviour is unchanged — 138 tests pass at 100% line coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the MPRGP implementation to make the outer loop’s two key decisions (projected-gradient stopping test and proportioning switch) clearer, and adds doctest-style examples to the public solver surfaces to document expected usage patterns (operator-based A, interpretation of converged vs kkt_violation, and solver interchangeability).
Changes:
- Decomposes the MPRGP loop into explicit move functions (
_cg_step,_expansion_step,_proportioning_step) plus a selector (_proportional_step), while keeping behavior/cost accounting consistent. - Adds doctest examples to
ActiveSetSolver,MPRGP, and thenncg.innermodule header to document typical usage and result interpretation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/nncg/solver.py | Adds an ActiveSetSolver doctest example demonstrating operator inputs, solution properties, and inner-solver swapping. |
| src/nncg/mprgp.py | Extracts MPRGP moves into dedicated functions and adds MPRGP doctest examples + consolidated step/product accounting. |
| src/nncg/inner.py | Adds module-level doctest examples showing how different inner solvers plug into the same outer loop and expected behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Aug 15, 2026
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.
What
Two changes to readability, no change to behaviour.
MPRGP loop decomposition.
_mprgpheld the conjugate-gradient, expansion and proportioning moves inline, so the two things the loop actually decides — the projected-gradient stopping test and the proportioning switch — were buried in the numerics of the moves themselves. Each move is now its own function over anIterate = (x, g, p)state:_cg_step— minimise within the current face; costs no Hessian product of its own._expansion_step— walk to the bound, then one projected-gradient move; the projection is non-linear, so this is the step that pays a second product._proportioning_step— release constraints along the chopped gradient._proportional_step— makes the feasibility choice between the first two and reports how many products it consumed._mprgpkeeps only the loop, the stopping test and the switch. The per-move counters collapse into a dict keyed by the move name the step returns, soproductsis accounted for at the one place that knows the cost.Doctests on
ActiveSetSolver,MPRGPand thenncg.innermodule header, covering the things a reader gets wrong first: thatAenters as aSymmetricOperatorand never as a bare array, thatkkt_violationscores the answer independently of the solver's ownconvergedflag, that the inner solvers are interchangeable without touching the outer loop, and that MPRGP's step counts sum toiterationswhilehessian_productsexceeds it.Verification
make test— 138 passed, 100% line coverage on every modulemake typecheck— ty and mypy strict, cleanmake fmt— all hooks passpytest --doctest-modules src/nncg— 7 passedNote for the reviewer
The doctests are not wired into any gate, so nothing runs them in CI and they can rot. Adding
--doctest-modulestopytest.iniwould fix that, butpytest.iniis Rhiza-owned — it belongs in the template, not in a hand-edit here. Flagging rather than fixing.🤖 Generated with Claude Code