Repository navigation
feat: warn when ADMM uses all its iterations without converging - #378
Draft
lachlangrose wants to merge 4 commits into
Draft
lachlangrose wants to merge 4 commits into
lachlangrose wants to merge 4 commits into
Conversation
Recipe: to_recipe_json / from_recipe_json do not store a feature's build parameters (nelements, interpolatortype, regularisation), so a model built with anything but the defaults rebuilds with the defaults and gives a different field (differences of 12 to 60 on a field of range about 560). The default-parameter case passes; the three others are strict xfails. Inequality: ADMM enforces the bounds with the arguments the existing builder test uses, but with its default arguments returns essentially the unconstrained solution (4 of 24 bounds violated, worst 0.056), and with solver cg the inequality data are dropped without a warning. A control (the unconstrained solution does violate bounds) and the working ADMM configuration pass; the default-ADMM case and the cg-warning expectation are strict xfails. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…olve solve_system column-scales the equality system (apply_scaling_matrix, on by default) and solves for y where c = S @ y, but passed the inequality matrix Q to ADMM unscaled. The bounds were therefore applied to y instead of to the field Q @ c. With scaling on, even a large penalty left bounds violated (admm_weight=10: 10 of 24). Pass Q @ S so Q @ y' = Q @ c. With this fixed, ADMM behaves like ordinary ADMM: admm_weight 1 and 10 now satisfy all 24 bounds (admm_weight=1 stops at iteration 90). The default penalty (0.01) is still too weak for the test scenario (primal residual 0.056 after 200 iterations against a tolerance of 0.0025) and the solver returns without a warning; that strict xfail stays, with its reason rewritten. The cg warning xfail is unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
admm_solve returned after nmajor iterations with no indication of whether it had converged, so an under-penalised or under-iterated solve left inequality constraints violated silently (with the default admm_weight=0.01 on the sparse dome, 4 of 24 bounds, worst 0.056). The loop now records how it ended. If it used every iteration without meeting the primal and dual tolerances it logs a warning with both residuals and their tolerances and suggests a larger admm_weight or nmajor. Stopping because the model stopped changing (model_update_tol, which the caller asks for) is not reported. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…residuals accurately The constant-norm polish re-solves with ADMM every iteration, so the warning repeated N+1 times. The first solve stays quiet when a polish follows and only the last polish iteration reports. When only the dual residual missed its tolerance the message no longer says the inequality constraints may be violated. warn_on_nonconvergence=False silences it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
admm_solvereturned afternmajoriterations without saying whether it had converged. #377 found that with the defaultadmm_weight=0.01the sparse dome test is still unconverged after 200 iterations (primal residual 0.056 against a tolerance of 0.0025, 4 of 24 bounds violated) and the solver gave no sign of it.Change
The major loop now records how it ended. If it used every iteration without meeting both the primal and dual tolerances it logs a warning:
model_update_tol, which the caller asks for).okflag returned bysolve_systemis stillTrue. I chose not to change that flag because other code treats it as "the solve ran".Tests (added to
test_inequality_enforcement.py)admm_weight;admm_weight=1.0converges, does not warn, and enforces the bounds.admm_weight=1e-4, nmajor=20warns and the bounds really are violated, so the warning is true;model_update_tolstop, no warning;nmajor=3(fewer thanmin_iterations=5, so it cannot be declared converged) warns.packages/loop_interpolation/tests: 618 passed, 46 skipped, 4 xfailed (includes the golden tests). ruff: no new findings.Notes
ADMM(0)raisesnelements must be greater than 0first (the error I saw in the earlier investigation). I did not change that.cgsolver silently dropping inequality data is a different path and is not addressed here (still an xfail).🤖 Generated with Claude Code