Only step as far as the linearization of the constraints is trusted. Fixes #1247 - #7
Open
BoykoNeov wants to merge 5 commits into
Open
Only step as far as the linearization of the constraints is trusted. Fixes #1247#7BoykoNeov wants to merge 5 commits into
BoykoNeov wants to merge 5 commits into
Conversation
PruneOrphans() and PruneRequestsAndConstraints() tag the requests and constraints that they mean to delete and then call RemoveTagged(), which removes every element whose tag is nonzero -- but neither of them clears the tags first. System::Solve() tags the constraints whose equations it couldn't satisfy, so that they can be listed for the user, and so the regeneration that immediately follows a failed solve deleted exactly those constraints: fail to solve a sketch once, and the constraints that failed are gone, along with any chance of correcting the value that caused it. Clear the tags before setting them. The request lists get the same treatment, not because anything tags a request today, but because they are pruned by the same tag-then-RemoveTagged() protocol, and would lose requests the same way if anything ever did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Group::Clear() frees every other dynamically-allocated member of a group, but not solved.remove -- the list that System::Solve() fills with the constraints whose equations it couldn't satisfy, and that FindWhichToRemoveToFixJacobian() fills with the ones it suggests removing to fix a redundant system. SolveGroup() empties that list before every solve, so it never grows during a session, but whatever is in it when the sketch is closed, the group is deleted, or the process exits is leaked. Nothing aliases the list: the undo stack zeroes solved when it shallow copies a group, and SolveGroup() clears the list directly rather than through Group::Clear(), so the report shown in the text window is unaffected. The leak is as old as the list, but it needs a solve to fail before there is anything to leak, and until the preceding commit's test nothing in the suite made one fail -- so the sanitizer build had nothing to find. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The interesting solver failures depend on the previous solution being the initial guess for the next solve, so they can't be reproduced by loading a file: the dimension has to be edited and the sketch re-solved, the way the GUI does it. Add a `solve` command that does that for a list of values and reports the solve result and the geometry after each one, and a `sweep` command that runs the same two step sequence over a range of values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A Newton step is only as good as the linearization of the constraint equations about the operating point, so where those equations are strongly curved the full step can land further from the solution than it started; and if it lands near a critical point of the equations, the step after that one is enormous and the geometry runs away to nowhere. That is what happens in issue solvespace#1247. The reporter's sketch reduces, after substitution, to one equation in the height h of a right triangle with a 100 mm base, cos(theta) = 100/sqrt(100^2 + h^2), whose derivative vanishes at h = 0. Changing the angle dimension from 60 to 30 degrees overshoots the root at h = 57.7 and lands at h = 4.2, right on top of that critical point; the next two steps are 161 and 349 mm, the triangle ends up out at +/-1.9e6 mm where the steps collapse to 1e-16, and the solve gives up with DIDNT_CONVERGE. Changing it to 40 degrees instead lands at h = 50, where the derivative is healthy, and converges in four iterations, which is why the failure looked threshold-shaped and dependent on the order of the edits. So keep a radius that we trust the linearization over, don't step further than that, and grow it only as steps keep working out: double it when a step reduces the residual, quarter it when one does not. The radius starts at half of the first Newton step, and since it doubles per accepted step it is back up to the full step immediately. If nothing along the step direction is an improvement, take the whole step anyway, exactly as before, so no step that we used to take is skipped and the iteration limit still catches it if that was a bad idea. IsReasonable() now shrinks the step instead of abandoning the solve. On a grid of 6480 (starting angle, target angle) pairs of the reporter's sketch, driven through the same edit-a-dimension-and-re-solve path as the GUI, this solves all 6480 where we used to solve 5662, and no pair that used to solve stops solving. It also stops mirroring the sketch: -30 degrees satisfies an angle constraint exactly as well as +30 does, and of the 5662 pairs both versions solve, the old code flips the triangle to the far side of the horizontal line on 619, where this steps to the nearer of the two solutions on all 619 and flips none. Accuracy is unchanged, worst error over the grid 3.82e-06 degrees against 3.69e-06 before. Halving the first step is what buys that, and it is the least of the divisors that does: with the whole first step the sketch still mirrors on 549 of the 619. Bigger divisors do no better on those pairs and cost more, so a half it is. The price is one extra Newton iteration when re-solving from an already-solved sketch, which is what happens on every frame of a drag: three iterations per small increment of a dimension where we used to take two. Over the whole of the existing test suite it is not measurable, 957 iterations against 956. Fixes solvespace#1247. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…point. The fixture is the sketch from issue solvespace#1247, and the cases drive it the way the GUI does, by changing a dimension's value and re-solving. That matters: the previous solution is the next solve's initial guess, so neither failure can be reproduced by loading a file that already has the failing value in it. test/debugtool.cpp grew a `solve` command for the same reason. The checks are relative rather than absolute because NewtonSolve() stops as soon as every equation's residual is under CONVERGE_TOLERANCE, which is dimensionless; the angle equation's residual is a direction cosine, so satisfying it to 1e-8 only pins this triangle's 173 mm height to a few times 1e-06 mm. Comparing the height itself against LENGTH_EPS passes or fails on which side of the tolerance the last step happens to land, which is not a property of the solver worth asserting. The second case checks the magnitude of the height only. From 85 degrees the first step that reduces the residual below its value there is one that crosses the horizontal line, so the triangle ends up mirrored; that is a solution where before there was none, but it is not the nearer of the two, and pinning the sign would assert a behaviour we would like to improve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I my opinion this is much better than #3 / solvespace#1748 I tried it and can not make it "flip" (with the few values I tried). |
|
@ruevs can you squash and merge these solver related commits? This is getting a bit confusing ;-) |
@phkahler there are two alternatives to fixing solvespace#1247:
In my opinion this (the second one above) is better. In addition there is #4, which fixes solvespace#1354 and is completely unrelated to this. There the LLM uses a linear algebra trick to avoid computing So what do you want me to squash/merge and where? |
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
3 participants
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.
Written by Claude Opus 5 — both this text and the code it describes; posted by @BoykoNeov.
An alternative to the line search in solvespace#1748 / #3, written in response to @ruevs' objection there: that the line search makes 59° → 25° land on the mirrored solution where master does not, and that the SolveSpace solver's predictability is one of the things it is valued for.
The two are either/or, not stackable — a trust region subsumes a backtracking line search, since both shrink the step when it fails to improve and the trust region merely carries the radius across iterations. solvespace#1748 is a good change and @ruevs has already tested it; this is offered as the better of two, for whoever is deciding, not as a request to close it.
Root cause of solvespace#1247, restated
tc7000's sketch reduces after substitution to
m=2, n=4, effectively one-dimensional in the triangle's heighth, withand
f′(0) = 0. Editing the angle 60° → 30° takes an undamped Newton step that overshoots the root ath = 57.735and lands ath ≈ 4.2, essentially on that critical point. From there the next steps are 161 mm and 349 mm, the geometry runs out to ±1.9e6 mm, the steps collapse to 1e-16 and the solve gives up with the residual still at cos 30°. 60° → 40° lands ath = 50, wheref′is healthy, and converges in 4 iterations. That is the whole "threshold-shaped, order-dependent" character of the report.The rank/pivot-tolerance explanation that looks obvious here is dead, killed with evidence rather than argued:
rankOkis true at every failure, andTestRank/CalculateRanknever fire.DIDNT_CONVERGEcomes straight out ofSystem::NewtonSolve().The fix
NewtonSolve()keeps a radius it trusts the linearization ofFover. Each step is clipped to that radius; the radius doubles when a step strictly reduces‖F‖₂²and quarters when one does not, for up to 12 tries. It starts at half the first Newton step. If nothing along the direction improves, the full step is taken exactly as the old code did, so no step master accepts is skipped.IsReasonable()failures now shrink the step instead of aborting the solve.EvalJacobian()andSolveLeastSquares()stay outside the retry loop, so a rejected trial step costs one residual evaluation — no Jacobian, no factorization.Why the divisor is 2: the headline result below holds for every divisor ≥ 2, so it is not a tuned constant; 2 is the cheapest such value, and 1 loses it (549 of the 619 flips come back). The whole-grid flip count is non-monotone in the divisor — 751, 615, 417, 170, 330, 204, 193, 198 for 1…512 — which is itself evidence that the residual flips are chaotic rather than tunable. An earlier version of this branch used 8, which is strictly dominated by 4 on both flips and cost.
Measured against master
A 1° grid of start × target angle over 5–85°, 6480 pairs, each driven through the GUI's edit-a-dimension-and-re-solve path. The repro is path-dependent — the previous solution is the next solve's initial guess — so it cannot be reproduced by loading a file with the target value already in it. Hence the
sweepcommand in the first commit.Scoped deliberately: zero flips wherever master has an answer at all. Across the whole grid 417 remain, every one of them inside the 818 pairs master cannot solve, where there is no previous behaviour to be surprising relative to.
Regression tests
test/core/solver/, fixture = tc7000's file from the issue.angle_step_over_critical_pointdoes 60° then 30° and checks the height's signed value, so it also pins the branch choice;angle_step_from_steep_solutiondoes 85° then 30° and checks magnitude only, because the trust region does land mirrored there (see the limitations below). Both fail on master'ssystem.cpp, at lines 53 and 71.The checks are relative —
CHECK_EQ_EPS(value / reference, 1.0)— on purpose, and there is a comment in the file saying why:CONVERGE_TOLERANCEis dimensionless while the angle equation's residual is a direction cosine, so satisfying it to 1e-8 only pins this triangle's height to a few times 1e-6 mm. Comparing the height itself againstLENGTH_EPStests the solver's luck. Master passes such an assert here by luck and is the less accurate of the two over the grid.Suite: 265 cases / 941 checks, Success!, Debug and Release. Master baseline 263 / 931.
Verification beyond the suite
test/constraint/large_dimensions(borrowed from Solve the Newton step without forming the normal equations. Fixes the original report and mesr's assembly from #1354 #4 as a probe, then removed) fails identically on master and here, so Large Dimensions result in Incompatible Constraints solvespace/solvespace#1354 is untouched.What to push back on
This is not a trajectory superset of master, and solvespace#1748 is. The line search engages only when the full step increases the residual, which is a complete safety argument by itself. This clips the first step unconditionally, so every solve takes a different path. The defence is entirely empirical: the 0-regression grid above, the byte-identical corpus, the unmeasurable suite cost, green Debug and Release. If that argument is worth more to you than the branch preference, solvespace#1748 is the more conservative change and I would not argue.
Cost in the drag regime.
NewtonSolve()runs once per mouse-move frame and the radius is a fresh local each call, so every frame re-pays the initial clip: +1 Newton iteration per frame (2.00 → 3.00 over 40 small monotone increments). That measurement is what changed the divisor from 8 to 2. Caveat: it is a dimension increment, sosys.draggedis empty and the reduced system is not identical to a real drag — the right regime, but a proxy. Asys.dragged.empty()conditional would buy exact parity while dragging; I left it out because it would ship an interactive path the harness cannot exercise even in principle, and it splits one constant into two UI-dependent regimes.It is not a guarantee. Animating a constraint value in small increments is continuation in the parameter, and under regularity that genuinely tracks a solution branch. A trust region limits step length in the state and tracks nothing — it exploits the same locality intuition without the theorem. 85° → 30° still crosses to the mirror, and the step that does it is a legitimate descent step a textbook ρ-ratio rule would also accept. Landing on the nearer root is a policy preference no branch-blind descent method gets for free.
The merit function mixes units — dimensionless cosine residuals against mm-valued distance ones, with the ANGLE
multfactor gaining the former by up to ~1000× near 0° and 180°. Harmless for a decrease-only acceptance test with a full-step fallback, but it is the kind of thing worth a second opinion.Note on the other solvespace#1247 work
The separate data-loss bug — a failed solve deleting the constraints it could not satisfy — is already upstream as solvespace#1744, merged. It is not in this branch and does not need to be.
If you want this, please fetch and fast-forward rather than using the merge button, so that fork master stays a straight ancestor of upstream's the way solvespace#1744 did.