refactor(mprgp): extract the three moves, and document the solvers by example - #89
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
This PR refactors the MPRGP solver implementation to make the three algorithmic “moves” (CG, expansion, proportioning) explicit functions over a shared (x, g, p) iterate state, while also adding doctest-style documentation examples to key public-facing solver entry points.
Changes:
- Extract MPRGP’s CG / expansion / proportioning moves into dedicated helpers and simplify
_mprgpto primarily own the loop, stopping test, and proportioning switch. - Introduce a move-keyed step counter map in MPRGP and keep Hessian product accounting explicit via the extracted step helpers.
- Add doctest examples to
ActiveSetSolver,MPRGP, and thenncg.innermodule to document the operator interface, KKT certification, and solver interchangeability.
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 doctest examples illustrating operator usage, KKT certification, fallback reporting, and inner-solver interchangeability. |
| src/nncg/mprgp.py | Refactors the MPRGP loop by extracting the three moves and selecting CG vs expansion via _proportional_step; adds doctest examples. |
| src/nncg/inner.py | Adds doctest examples showing how different inner solvers affect the active-set outer loop and how iteration counts behave. |
💡 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.
What
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.
Iterate = (x, g, p)state._proportional_stepmakes the feasibility choice between the CG and expansion moves and reports the Hessian products it consumed._mprgpkeeps only the loop, the stopping test and the switch; the per-move counters become a dict keyed by the move name the step returns.Docs
Doctests added to
ActiveSetSolver,MPRGPand thenncg.innermodule, covering:iterations.Verification
Behaviour is unchanged — 138 tests pass at 100% line coverage.
🤖 Generated with Claude Code