Conversation
…ying on it post-hoc success_tolerance fed nlopt's early-out AND doubled as the post-solve success gate, so a large tolerance let trajectories violate real joint/collision limits (esp. collision, since it's a raw world-unit distance there). Now limits/geometry are tightened once per solve (not per-iteration) so nlopt's unchanged early-out always lands inside the true bounds. Also restores the retry-loop break so a good early try isn't overwritten by a worse later one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WjNkTbJJp46EMVXtVTR9oZ
…ainst stop_to_stop pins the first/last control points, so the position rows of the boundary segments are constant in the decision variables -- gradient exactly 0. Tightening the joint range by 1+tol moves the bound inside an endpoint the solver cannot move. Measured, bookshelf_small_ur5/0001 joint 5 (limits +/-3.1416, endpoint 3.14): -0.00050926 -> +0.00948565 with |grad| = 0, the only such row in 245. SLSQP linearises it as 0.00948565 <= 0, false for every step: the QP is infeasible at every iterate and no step fixes it. Explains what did not fit before: damage binary in tol (c = tol - 0.000507, so 0.01 and 0.001 equally poisoned), collision bake irrelevant (10mm -> 1mm bought 0.7pp), ROUNDOFF_LIMITED 0.2% -> 72.6%, max_con floor at 0.00949, max_con 9.7e18. validate_task passes on true geometry first, so the task is certified valid and then handed to the solver unsatisfiable. MBM v4, 7 scenes, 18183 rows/arm, referee success, 0 false claims either arm: native 38.9% -> 56.9% cascade 37.7% -> 66.4% Also: - single tolerance. Accept gate was success_tolerance*2, spending exactly twice the margin the tightening buys, so the guarantee never held: it halved the pre-fix 2cm allowance to 1cm, never zeroed it. nlopt's own early-out already fires at con_tol = tol, which implies d_true >= 0. - collision_buffer (metres) replaces a dimensionless tol used as a length. Caller states a length; scale = tol/buffer is derived in the snapshot and geometry grows buffer/2 per object, so the algebra cancels exactly: c = -(d - buffer)(tol/buffer) < tol <=> d_true > 0, any tol, any buffer. Verified by bisection: sphere gives exactly 0.000000000 m clearance at the gate for buffer 0.5mm-10mm; box +19.8%, conservative (extent growth on 3 axes grows support by e(|nx|+|ny|+|nz|) >= e). buffer is the SLACK ALLOWANCE, not the resulting clearance. Old tol/2 bake made 346/551 MBM endpoints unattainable; at 1mm it is 2/551. - tool_speed used bound_constraint(v, 0, vmax): a two-sided formula on a one-sided quantity. 0 at v=0, so a stationary tool reads as exactly active, and below half speed the gradient is negative -- the solver is rewarded for speeding up. In validate_task it was called with value_min = -INF_REAL -> NaN, and NaN > 0 is false, so that check never fired. -> abs_constraint, 10 sites. - Optimization::tighten_for_tolerance, an ablation switch. Snapshot taken, nothing modified, restore a no-op. Only way to attribute a change to the tightening rather than a neighbouring commit; needed here, every earlier number compared two different commits (worth ~3.3pp). STILL OPEN: same trap exists for velocity/acceleration. Task(const Matrix&) takes arbitrary boundary velocity/acceleration; stop_to_stop only happens to zero them, and a pinned 0 is satisfied (-1) so its zero gradient is harmless. A task pinning a NONZERO boundary velocity near the limit reproduces this exactly. No MBM task does, so this benchmark cannot see it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gu1SXrjfSfG83XmHzyxhQy
Three properties, none of which the suite could catch before. test_optimization_tolerance_no_penetration asserts max_con < 1e-6 on RESTORED geometry in a scene whose solve ends far inside the constraints, so it passed both under the 2*tol gate and with an infeasible QP. 1. No constraint row may be violated with a zero gradient. Verified to FAIL on the pre-fix code (16 offenders = 8 draws x 2 pinned endpoints), pass after. Needs a +/-pi robot: make_UR5e() is +/-2pi, where an endpoint at 3.14 sits mid-range and the bug cannot appear. Asserted over 8 draws because init_guess_segments() is random -- the violated-row COUNT varies 57-151 run to run, the invariant does not. 2. c < tol implies d_true > 0, swept through contact so it is not vacuous (both_sides > 0 asserts the sweep really crossed). 3. restore_from_tolerance puts collision_scale back to 1, and position is never tightened. Not pinned: the gate CONSTANT itself. (2) tests the algebra the gate relies on, but a regression to success_tolerance*2 would only be caught by a solve that lands between tol and 2*tol, which no fixture guarantees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gu1SXrjfSfG83XmHzyxhQy
CHECK(num_tries == 1) failed on the Windows CI build (2 == 1) and is flaky on Linux too, measured 1/100. Cause is ours: the accept gate became a single success_tolerance (was success_tolerance*2), so a marginal first attempt can now legitimately fail and the loop retries with a random guess. The regression this test guards is "num_tries always equalled max_tries" -- the break on first success having been commented out. `== 1` conflated that with "the first try always succeeds", which is a claim about the solver, not the loop. Now asserts 1 <= num_tries < max_tries. 0/200 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gu1SXrjfSfG83XmHzyxhQy
Guess::random is the default and seeds a thread_local mt19937 from std::random_device, so every `CHECK(result.success == true)` was really asserting "this task solves from an arbitrary random guess" -- a claim about the solver, not about the behaviour under test. Measured before this change: test_optimization_basic 9 / 100 failures test_optimization_tolerance_no_penetration 1 / 100 (the Windows CI failure) Both were 0 before the accept gate became a single success_tolerance, so the flakiness is a real consequence of that change surfacing in fixtures that never exercised the position bug (make_UR5e is +/-2pi, endpoints sit mid-range). test_helper.hpp gains straight_line_guess(): a straight line from start to goal in control-point space plus a fixed T. Layout per Bspline::compute_control() -- joint-major, each joint's free interior control points (i = 3 .. n_ctrl-4) consecutively, T last; valid for a fully specified task, which stop_to_stop gives. After: 0/150 on all three, 19/19 suite. The contract test drops its 8 random draws for one deterministic guess: a zero-gradient row does not depend on the decision variables by definition, so its value is identical for every x. Re-verified it still catches the bug -- 30/30 failures with the position tightening reintroduced, 0/30 with the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gu1SXrjfSfG83XmHzyxhQy
…os/tight_validation
Comments cited external problem ids and result numbers blast has no knowledge of. Restated in terms of the library's own quantities; test helper renamed mbm_like_UR5e -> UR5e_narrow_limits.
collision_buffer was a new public field with no mention in the guides. Also cuts the over-long comments added with the tightening work down to the mechanism.
…os/tight_validation
Nikos-d
marked this pull request as ready for review
August 29, 2026 17:00
gallantandre
reviewed
Sep 5, 2026
gallantandre
left a comment
gallantandre
left a comment
Member
There was a problem hiding this comment.
Just a couple of questions before merging. Let me know what you think and we'll merge soon.
| } | ||
|
|
||
| opt->guess = start_guess; // reset to original | ||
| restore_from_tolerance(opt, tolerance_snapshot); |
Member
There was a problem hiding this comment.
not sure what this does. please add comment.
Comment on lines
+32
to
+56
| // Guess::random (the default) seeds a thread_local mt19937 from std::random_device, | ||
| // so any test asserting `result.success == true` is really asserting "this task | ||
| // solves from an arbitrary random guess", which is a statement about the solver, | ||
| // not about the behaviour under test. Measured on the UR5e fixtures, that fails | ||
| // ~9/100 -- and it was 0/200 before the accept gate became a single | ||
| // success_tolerance, so the flakiness is real and CI-visible, not hypothetical. | ||
| // | ||
| // Layout comes from Bspline::compute_control(): joint-major, each joint contributing | ||
| // its free interior control points (i = 3 .. n_ctrl-4) consecutively, then T last. | ||
| // Valid only for a fully specified task (no NaN boundary values) -- which is what | ||
| // Task::stop_to_stop produces. | ||
| inline Array straight_line_guess(const Optimization& opt, const Array& start, | ||
| const Array& goal, real total_time = 2.0) { | ||
| Array x(opt.bspline.x_len(opt.task)); | ||
| const u32 n_free = opt.bspline.n_ctrl - 6; | ||
| Assert(x.size == (u32) opt.manip.n_joints * n_free + 1); | ||
| u32 k = 0; | ||
| for (int j = 0; j < opt.manip.n_joints; j++) | ||
| for (u32 i = 0; i < n_free; i++) { | ||
| const real a = (real) (i + 1) / (real) (n_free + 1); | ||
| x[k++] = start[j] + a * (goal[j] - start[j]); | ||
| } | ||
| x.back() = total_time; | ||
| return x; | ||
| } |
Member
There was a problem hiding this comment.
why not in the actual blast? It's something actual users might want to use for their own tests and it doesn't take too much space. We have other initial guess generators.
- Added straight_line initial guess to blast - Added comments for clarity - Cleaned up relevant test
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
2 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.
instead of relying on it post-hoc
success_tolerance fed nlopt's early-out AND doubled as the post-solve success gate, so a large tolerance let trajectories violate real joint/collision limits (esp. collision, since it's a raw world-unit distance there). Now limits/geometry are tightened once per solve (not per-iteration) so nlopt's unchanged early-out always lands inside the true bounds. Also restores the retry-loop break so a good early try isn't overwritten by a worse later one.