fix(exact): make the rcond_free conditioning guard opt-out (#32) - #33
Merged
Conversation
The Exact inner solver ran op.rcond_free() -- an O(|F|^3) symmetric eigendecomposition -- before every free-block solve. On the plain solve() path each outer step visits a different free set, so the 0.4.1 memo never hits and the guard fires every step, where it can cost several times the Cholesky solve it precedes (~3.6x on a dense 250-dim free block; a 500-var direct solve drops from ~21ms to ~6ms). The guard is also redundant when solve_free already fails loudly on a rank-deficient block (cvx.linalg.cholesky_solve's Cholesky->LU path). Add check_conditioning: bool = True on Exact; default keeps current behavior, check_conditioning=False skips the eigendecomposition and lets solve_free surface any singularity itself. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the expensive rcond_free conditioning guard in nncg.inner.Exact optional via a new check_conditioning flag, enabling callers to skip an O(|F|^3) eigendecomposition when they prefer raw solve_free performance.
Changes:
- Add
Exact(check_conditioning: bool = True)and gate the conditioning check on this flag. - Expand
Exact’s docstring to explain the performance motivation and how to opt out. - Add a regression test ensuring
rcond_freeis never called whencheck_conditioning=Falseand that solutions match the guarded path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/nncg/inner.py | Adds check_conditioning flag and conditionally skips the rcond_free guard; updates documentation accordingly. |
| tests/test_operators.py | Adds a test that counts rcond_free calls and verifies correctness when conditioning checks are disabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+239
to
+243
| can cost several times the Cholesky solve it precedes. The guard is also | ||
| redundant when ``solve_free`` already fails loudly on a rank-deficient block | ||
| (e.g. ``cvx.linalg.cholesky_solve``'s Cholesky→LU fallback). Set | ||
| ``check_conditioning=False`` to skip it and let ``solve_free`` surface any | ||
| singularity itself. |
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 #32.
Problem
Exact.solveranop.rcond_free(idx)— anO(|F|³)symmetric eigendecomposition (np.linalg.eigvalsh) — before every free-block solve. On the plainActiveSetSolver(Exact()).solve(...)path, every outer step visits a different free set, so the 0.4.1 single-slot memo never hits and the guard fires on every step. For a dense free block it costs several times the Cholesky solve it precedes.Measured (n=500, κ=1e4,
DenseOperator): the guard'seigvalshis ~3.6× thecholesky_solve, and a full direct solve drops from ~21 ms to ~6 ms (3.5×) with the guard off — identical solution (max|Δx| = 0).The guard is also redundant when
solve_freealready fails loudly on a rank-deficient block (cvx.linalg.cholesky_solve's Cholesky→LU fallback).Change
Add
check_conditioning: bool = TrueonExact:solve_eqamortization from Exact inner solver recomputes rcond_free p+1 times per free set in solve_eq (regression of #18) #30 are untouched.Exact(check_conditioning=False)skips the eigendecomposition and letssolve_freesurface any singularity itself — for callers who know their free blocks are well-conditioned (e.g. ridged/Gram operators) and want the raw structured solve.Tests
test_exact_check_conditioning_false_skips_rcond_guard: assertsrcond_freeis called 0 times when disabled, the solve still converges to the planted optimum, and the result matches the guarded solve bit-for-bit.🤖 Generated with Claude Code