test(solver): assert the max_outer contract instead of a magic outer count - #49
Merged
Merged
Conversation
…47) The two cap tests hard-coded `res.outer == 1` for `max_outer=1`. Replace each with a contract test that measures the uncapped outer count from the public `Result` and asserts the loop stops at exactly `cap` steps for every cap short of convergence. Stronger (covers all caps, not one), reads only the public surface, and robust to backend-dependent iteration-count drift. The remaining count assertions are left as-is by design: `.inner` checks are already relative/bounded, the warm-start `outer == 1` are documented single-outer-step guarantees, the trajectory equalities compare two solvers (inexactness lemma), and the _CountingOperator call-counts are justified white-box regressions for #30/#32 (call patterns not observable through Result). tests/test_paper/test_results.py was already fully behavioural and is unchanged. 102 tests pass at 100% coverage. 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 updates the solver test suite to avoid brittle, hard-coded outer-iteration counts by asserting the public Result.outer / Result.converged max_outer contract instead of expecting a specific outer == 1 outcome for max_outer=1.
Changes:
- Reworked the bound-constrained
max_outercap test to measure the uncapped outer count and verify truncation behavior for every smaller cap. - Reworked the equality-constrained
max_outercap test in the same way. - Expanded docstrings to explain why the new assertions are more robust to backend-dependent iteration drift.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+172
to
+174
| full = ActiveSetSolver(inner=CG()).solve(op, b).outer # uncapped step count | ||
| assert full > 1 # a non-trivial support needs more than one step to reach | ||
| for cap in range(1, full): |
Comment on lines
+578
to
+580
| full = ActiveSetSolver(inner=CG()).solve_eq(op, b, b_eq, c_eq).outer # uncapped step count | ||
| assert full > 1 | ||
| for cap in range(1, full): |
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.
Addresses #47.
Audit first
I audited every iteration-count / trajectory assertion in the two files the issue names. Most were already compliant:
tests/test_paper/test_results.py— already fully behavioural:res.outer < 10(bound),res.fallback == 0(dormant-fallback claim),growth < envelope(relative √κ bound),res.freesupport checks. Unchanged..innerassertions intest_solver.py— already relative/bounded (r_pcg.inner <= 0.7 * r_cg.inner,warm.inner < cold.inner).outer == 1— documented single-outer-step guarantees (warm start from an already-optimal free set converges in one step); exact by design, not implementation drift.r_cg.traj == r_ex.traj) — compare two solvers to each other (the inexactness lemma), not a golden internal sequence._CountingOperatorcall-counts — justified white-box regressions for Exact inner solver recomputes rcond_free p+1 times per free set in solve_eq (regression of #18) #30/Exact inner solver: unconditional rcond_free eigendecomposition dominates the direct-solve cost (~3.6x the solve it guards) #32; those call patterns aren't observable throughResult, so they stay.What changed
The one genuinely brittle pattern was the two cap tests hard-coding
res.outer == 1formax_outer=1— a magic expected count. Each is now a contract test:This is strictly stronger than the original (covers every cap below convergence, not just
1), reads only the publicResultsurface, and is robust to backend-dependent iteration-count drift because the reference count is measured, not pinned.Verification
make testmake fmtAcceptance criterion (#47): iteration-count assertions reference only the public
Resultsurface and express contracts/bounds rather than exact internal state; the paper's claims are still enforced; suite green at 100% coverage.🤖 Generated with Claude Code