feat: factorization reuse and anti-cycling (#27, #29) - #54
Merged
Conversation
#27 — §13.2's three updates. What is kept across an iteration is a QR of `W.T`, not the saddle-point matrix: adding a constraint appends one column, removing one deletes a column, and the tangent row moving replaces a column, so all three of §13.2's cases are operations on the same object. #23 made the (1,1) block move every iteration a cone is active, which would have made a kept KKT factorization mostly stale; `W.T` has no such problem. Which case happened is recognized by diffing the matrices rather than by being told. §4.1's loop has six paths that change a working set and threading a stateful object through all six is how a stale factorization gets used. The first version compared rows in step and handled exactly one change; the loop's ordinary iteration makes two, since the step that reached a blocking row also moved the tangent, and the cone's rows come last so an inserted bound shifted every later row. A real diff over the rows' bytes fixed it: 26% of solves still factorizing became 1.5%. Measured. Families 98.9% -> 1.4%, randomized sweep 98.7% -> 1.5%; a whole solve typically factorizes twice. Wall-clock is the interesting half and goes the other way at these sizes: a column update against a full (n,n) Q costs O(n^2) while a fresh QR of an (n,m) matrix costs O(n m^2), so updating wins only once m^2 exceeds n. The classical argument assumes a working set comparable to the problem; an active-set method on a portfolio runs with m well below n. Measured 0.98x at n=300 and 1.12x at n=500. #29 — §17.2's four remedies and §8.2's hysteresis. Bland's rule and the guard that arms it after a working set is returned to more than three times; the merit safeguard; multiplier tolerances, already in place since #11; and eps_on < eps_off, with the geometric release clause that complementarity already implied. The cycle that was actually there was none of those. Reuse exposed it: the revisit counter had been counting iterations *held* on a set rather than returns *to* it, which reported nine hundred revisits for a slow solve and hid a genuine two-state cycle. Fixed, the worst randomized instance turned out to alternate APEX and INACTIVE 486 times — #24's branch releases an apex it cannot justify, the released direction cannot be travelled, so the step moves nothing and §7.3 reads the same unchanged geometry and puts the factor straight back. The fix is to not re-derive a status from geometry that has not changed. Worst returns across 200 randomized instances: 486 -> 2. Also a no-progress rule, which belongs here for the same reason: an iteration that moves the iterate by nothing and changes nothing about the working set has achieved nothing and repeating it will achieve nothing again. That, rather than a refitted threshold, is what the tail needed — `_STATIONARY` makes no difference at 1e-10 or 1e-9 with it in place, so it is left where it started. All eleven families optimal; randomized non-optimal 8 of 200, none cycling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 10 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6 tasks
This was referenced Sep 4, 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.
Closes #27, #29. Wave 8, on top of #53.
#27 — §13.2's three updates
What is kept across an iteration is a QR of
W.T, not the saddle-point matrix. Adding a constraint appends one column, removing one deletes a column, and the tangent row moving replaces a column — so all three of §13.2's cases are operations on the same object. That choice is forced by #23: the Lagrangian curvature makes the(1,1)block move every iteration a cone is active, so a kept KKT factorization would be mostly stale.W.Thas no such problem, and a QR of it is exactly what the null-space route needs.§13.2 singles out the third case — "the SOC tangent changes continuously with
x, so this part is more subtle than ordinary linear constraint updates". The subtlety is real but it is about frequency, not kind: the row changes every iteration the cone is active, soreplaceruns far more often than the other two, and the linear structure around it is never refactorized.Which case happened is recognized by diffing the matrices, not by being told. §4.1's loop has six paths that change a working set, and threading a stateful object through all six is how a stale factorization gets used by mistake.
That diff had to be a real one. The first version walked the two matrices in step and handled exactly one change — but the loop's most ordinary iteration makes two (the step that reached a blocking row also moved
x, and so moved the tangent), and the cone's rows come last in the layout, so an inserted bound shifted every later row and the walk read the shift as a cascade of deletions.difflibover the rows' bytes fixed it: 26% of solves still factorizing became 1.5%.Measured
A whole solve typically factorizes twice.
Wall-clock is the interesting half, and at these sizes it goes the other way. A column update against a full
(n,n)orthogonal factor costsO(n²); a fresh QR of an(n,m)matrix costsO(n m²). So updating wins only oncem²exceedsn— and the classical argument for factorization updates quietly assumes a working set comparable in size to the problem, while an active-set method on a portfolio runs withmwell belown. Measured on the box family: 0.98× at n=300, 1.12× at n=500. Both halves are recorded indocs/development/architecture.md, because #34 has to report this honestly and a reader deciding whether to adopt the technique needs it.#29 — §17.2's four remedies, §8.2's hysteresis, and the cycle that was actually there
All four remedies: Bland's rule (
lexicographic_candidate) and theGuardthat arms it after a working set is returned to more than three times; the merit safeguard; multiplier tolerances, in place since #11 and left there; andeps_on < eps_off, with a geometric release clause that complementarity already implied — a strictly interior slack forcesw = 0, which was already §7.4's "no active normal" test.None of them was the problem. #27 exposed the real one. The revisit counter had been counting iterations held on a working set rather than returns to it, which reported 900 revisits for a solve that was merely converging slowly — and hid a genuine cycle underneath. Corrected, the worst randomized instance turned out to alternate
APEXandINACTIVE486 times: #24's branch releases an apex it cannot justify, the released direction cannot be travelled (result 4 — arithmetic, not tolerance), so the step moves nothing, and §7.3 reads the same unchanged geometry and puts the factor straight back.The fix is one line of principle: do not re-derive a status from geometry that has not changed.
Also a no-progress rule, which belongs here for the same reason: an iteration that moves the iterate by nothing and changes nothing about the working set has achieved nothing, and repeating it will achieve nothing again. Both halves are required — a zero-length step is how a constraint gets added. That, rather than a refitted threshold, is what the convergence tail needed: with it in place
_STATIONARYmakes no difference at1e-10or1e-9, so it is left where it started rather than tuned.Done when
All rhiza gates green, 100% coverage, 1077 tests.