Hoist the free-set restriction out of the inner CG loop (closes #12) - #13
Merged
Conversation
_free_matvec returned lambda v: op.apply_free(idx, v), so every inner CG iteration re-gathered the operator's storage (two fancy-indexed copies of the Gram factor per matvec): identical iteration counts, roughly 5-15x per-iteration wall clock. It now builds the pre-sliced free-block operator once via restricted(idx) (cvx-linalg >= 0.10, Jebel-Quant/linalg#89) and returns its plain matvec, falling back to apply_free for older releases or backends without a pre-sliced form. Same root cause as fast_minimum_variance#72 (fixed there in #73). Closes #12. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the inner CG matvec factory by hoisting free-set restriction construction out of the per-iteration path when the backend supports restricted(idx), while retaining a legacy fallback via apply_free.
Changes:
- Update
_free_matvecto preferop.restricted(idx).matvecand fall back onapply_freewhen unavailable or unimplemented. - Add a new test module covering both the hoisted path and the fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/nncg/solver.py |
Hoists free-set restriction via restricted(idx) when supported; preserves apply_free fallback. |
tests/test_free_matvec.py |
Adds coverage for restricted-capable operators and fallback scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+30
to
+31
| op, a = _dense_pair(1) | ||
| idx = np.array([1, 3, 4]) |
Comment on lines
+46
to
+47
| op, a = _dense_pair(2) | ||
| idx = np.array([0, 4]) |
Comment on lines
+98
to
+103
| restricted = getattr(op, "restricted", None) | ||
| if restricted is not None: | ||
| try: | ||
| return restricted(idx).matvec | ||
| except NotImplementedError: | ||
| pass # backend without a pre-sliced form; fall back below |
- cast restricted(idx).matvec through MatVec so mypy stops flagging the getattr-sourced Any as a no-any-return - add the missing _dense_pair docstring (docs-coverage gate is 100%) - adopt ruff's fixes in test_free_matvec (unused op -> _op, drop stale noqa) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
`_free_matvec` now builds the pre-sliced free-block operator once per working set via `restricted(idx)` (Jebel-Quant/linalg#89) and returns its plain `matvec`; the old `apply_free`-per-iteration path remains as a fallback for cvx-linalg releases without `restricted` and for backends that inherit its raising default.
Effect: identical iteration counts and certificates, ~5-15x less per-iteration wall clock (the same anti-pattern as fast_minimum_variance#72/#73). On the minvar paper's balance study (S&P 500, n=494, sleeves p in {1,4,8}) the solve times drop accordingly; tables regenerate with unchanged iteration columns.
Tests: hoisted path returns the restricted matvec, fallback works for legacy operators (no `restricted`) and for the raising default. Suite passes against both the released cvx-linalg and linalg#89.
🤖 Generated with Claude Code