Repository navigation
Conversation
…rse. NewtonSolve() took the whole Newton step on every iteration, but that step is only as good as the linearization of the constraint equations about the current operating point. In tc7000's sketch from solvespace#1247, a right triangle with one leg dimensioned and the hypotenuse at an angle, the step from the 60 degree solution towards the 30 degree one overshoots the root at h = 100*tan(30) = 57.7 mm and lands at h = 4.2 mm, right next to the critical point of the angle equation f(h) = 100/sqrt(100^2 + h^2) - cos(30 degrees), f'(0) = 0 where the direction cosine is stationary. The step from there is enormous, and after a few more the triangle's vertices are 1.9e6 mm apart, the steps collapse to nothing, and the solve gives up with a residual of cos(30 degrees) left over. That is why the failure is threshold shaped: 60 to 40 lands at h = 50 mm and converges fine, 60 to 30 lands on the critical point and runs away. The Newton step is a descent direction for |F|^2, so if the full step increases the residual then a short enough step along the same direction decreases it. Backtrack, halving up to eight times, and if nothing along that direction is an improvement then take the full step anyway, so that we never reject a step that the old code would have taken. Sweeping this sketch over 138 pairs of before and after angles, the 32 pairs that used to fail all solve now, and none that used to solve stopped solving. Also add a regression test, with tc7000's sketch as its fixture, that checks the geometry after each solve and not just the result code, since the angle equation is even in h and could be satisfied by the mirrored triangle. Fixes solvespace#1247. 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>
Member
Author
|
Rebased on master here #1748 |
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.
Based on BoykoNeov#3. Original PR text from Claude Opus 5:
Fixes #1247 — @tc7000's sketch where changing an angle dimension from 60° to 30° reports "didn't converge", while 60° → 40° is fine, and the same 30° works if you approach it in smaller steps.
It is not a rank or redundancy problem
That was my first hypothesis, and it is wrong.
rankOkis true at every failure, andTestRank/CalculateRanknever fire on this model — theDIDNT_CONVERGEcomes straight out ofSystem::NewtonSolve(). Worth saying explicitly, because the symptoms (a threshold that depends on the previous value, order dependence) look exactly like a redundancy problem and it's a natural place to start digging.Root cause: an undamped Newton step lands on a critical point
After substitution this sketch reduces to
m = 2, n = 4, effectively one-dimensional in the heighthof the triangle, withf'(0) = 0— the function is flat at the origin. Solving for 30° means findingh = 57.735, and from the 60° solution the full Newton step overshoots it and lands ath ≈ 4.2, right on top of that critical point. With the derivative there near zero, the next steps are 161 mm, then 349 mm, and the geometry runs out to ±1.9e6 mm, where the parameters are so large that steps collapse to ~1e-16 and the solver gives up with the residual still atcos 30°.60° → 40° lands at
h = 50, where the derivative is healthy, and converges in four iterations. That is the whole "threshold-shaped, order-dependent" story: whether the full step happens to clear the flat spot.The fix
A backtracking line search in
System::NewtonSolve()(src/system.cpp): tryrelax = 1, ½, ¼ … 1/128, and accept the first that strictly reduces‖F‖₂². If none does, take the full step exactly as before — so no step the old code would have taken is ever rejected, and a model that converges today cannot start failing because of this.IsReasonable()failures now shrink the step instead of aborting the solve outright. (Note for anyone reading that function: it is misnamed upstream — it returns true when the value is unreasonable.)Regression test
test/core/solver/angle_step_over_critical_point— set 60°, then 30°, and check both solve and that the triangle really has the right height, not just that a result code came back OKAY. Fails at the 30° step without the fix.The repro is path-dependent: the previous solution is the next solve's initial guess, so it cannot be reproduced by editing the
.slvsand reloading. The second commit therefore teachestest/debugtool.cppto load a sketch and then set a dimension and re-solve repeatedly (solve), and to sweep a range of start/target pairs (sweep). That is what produced the numbers above and it is useful for any solver issue of this shape.Verification
src/system.cppreverted, the new test fails at the 30° step.Concerns you may want to raise
‖F‖₂²sums dimensionless cosine residuals from angle constraints — which themultfactor can gain up to ~1000× near 0°/180° — together with mm-valued distance residuals. This is harmless for a decrease-only acceptance test with a full-step fallback (the worst case is that a good step is judged "not an improvement" and we fall back to exactly today's behaviour), but it is not a principled merit function, and a residual scaling pass would be a real improvement. I did not attempt one.This PR is independent of my other three and can be taken on its own. In particular, note that a failed solve on this branch still deletes the constraints that failed — that is a separate bug with its own PR, deliberately not mixed in here.
Please fetch and fast-forward this branch rather than using the merge button on my fork — that keeps it a clean fast-forward for upstream.
🤖 Generated with Claude Code