Repository navigation
feat: warn when inequality constraints are supplied to a solver that ignores them - #379
Draft
lachlangrose wants to merge 8 commits into
Draft
lachlangrose wants to merge 8 commits into
lachlangrose wants to merge 8 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>
…ignores them Only the admm solver (and the native solver) use inequality constraints. With cg, lsmr or a callable solver they were dropped without a word, including the l / u columns of model data that GeologicalModel turns into inequality constraints and then solves with cg by default. solve_system now logs a warning, naming the solver and the number of ignored rows, when the inequality system has rows and the solver is not admm. Nothing else changes. The cg xfail from the inequality test file is now a passing test; the default-penalty xfail stays. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… installed 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>
…or, not on every solve 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.
Only the
admmsolver (and the native solver) use inequality constraints. Withcg,lsmror a callable solver they were dropped without a word.This is not just a builder-API corner.
GeologicalFeatureBuilderturns thel/ucolumns of model data into inequality constraints, andGeologicalModelsolves withcgby default, so inequality data in a model's data frame was silently ignored.Change
solve_systemlogs a warning when the assembled inequality system has rows and the solver is notadmm:It names the solver (
'custom'for a callable) and the number of rows. The native solver returns earlier insolve_systemand handles inequalities itself, so it is not affected. Nothing else changes: no result changes, no new failure.Tests (
test_inequality_enforcement.py)cgxfail from test: pin the recipe and inequality-constraint problems (strict xfail) #374 is now a passing test (it was my expectation, not a documented contract; the message says what to do).'custom');admmdoes not warn;cgwith no inequality rows does not warn.l/ucolumns added to the Claudius model data, default solver,model.update()warns about 2 rows.packages/loop_interpolation/tests: 623 passed, 46 skipped, 3 xfailed;tests: 731 passed, 6 skipped, 3 xfailed (the 3 are the recipe xfails from test: pin the recipe and inequality-constraint problems (strict xfail) #374, which this branch does not fix). ruff: no new findings.Not done
It stays a warning. Switching to
admmautomatically when inequality data is present would be a behaviour change (andadmm's default penalty is still weak, see #377 / #378), so I left that to a decision.🤖 Generated with Claude Code