Skip to content

fix: scale the inequality rows with the equality system in the ADMM solve - #377

Draft
lachlangrose wants to merge 2 commits into
masterfrom
fix/admm-inequality-defaults
Draft

lachlangrose wants to merge 2 commits into
masterfrom
fix/admm-inequality-defaults

Conversation

@lachlangrose

Copy link
Copy Markdown
Member

Draft, stacked on #374 (the xfail tests). Its diff against master includes #374's commit until that merges; the change here is the last commit.

Diagnosis and a fix for part of the ADMM inequality problem reported in #374.

Cause 1 (fixed here): the inequality rows were not scaled with the equality system

solve_system column-scales the equality system (apply_scaling_matrix, on by default) to A @ S and solves for y with c = S @ y. The inequality matrix Q was passed to ADMM unscaled, so ADMM enforced lower <= Q y <= upper on the scaled variable instead of on the field Q @ c. The matrix itself was right (Q @ c equals evaluate_value to 1e-16, no empty rows).

Evidence (sparse dome scenario, 24 bounds, ADMM defaults except the penalty):

admm_weight scaling on, before scaling on, after scaling off
1.0 4/24 violated (worst 0.103) 0/24 0/24
10.0 10/24 violated (worst 0.220) 0/24 0/24

The fix (Q @ scaling_matrix after _preprocess_main_system) makes scaling on behave like scaling off.

Cause 2 (not fixed): the default penalty is too weak, and non-convergence is silent

With cause 1 fixed ADMM behaves like ordinary ADMM. The default admm_weight=0.01 does not converge in the default nmajor=200 for this scenario (primal residual from the solver's history, tolerance 0.0025):

setting iterations run final primal residual bounds violated
admm_weight=0.01, 200 200 0.056 4/24 (worst 0.048)
admm_weight=0.01, 1000 1000 0.043 3/24
admm_weight=0.01, 5000 5000 0.0145 2/24 (worst 0.013)
admm_weight=0.1 150 (stopped) 0.0025 2/24 (worst 0.002, within tolerance)
admm_weight=1.0 90 (stopped) 0.0000 0/24

It reaches nmajor without meeting its tolerance and returns without a warning. I did not change the default penalty: it affects every ADMM user and I have only tested one scenario. A sensible follow-up is to warn when nmajor is reached unconverged, and/or revisit the default. (adaptive_rho=True did not change the result at the default penalty; I did not investigate why.)

Why the argument dependence looked arbitrary

nmajor=10, admm_weight=0.01, maxiter=50 (the existing builder test's arguments) enforces the bounds only by accident: it is a truncated, unconverged solve (maxiter=100 with the same nmajor violates the bounds again). All converged configurations before the fix returned the same infeasible answer regardless of inner solver (lsmr/cg), tolerances (1e-12) or warm start. admm_weight was a red herring in my earlier note; the three-argument combination was.

Tests

test_inequality_enforcement.py:

  • new test_admm_enforces_the_bounds_with_and_without_column_scaling (scaling on/off x admm_weight 1 and 10). With the fix reverted, exactly the two scaling-on cases fail.
  • the default-penalty xfail stays, with its reason and the module docstring rewritten to the diagnosis; the cg warning xfail is unchanged.
  • packages/loop_interpolation/tests (including the golden tests): 612 passed, 46 skipped, 4 xfailed (before: 606 passed on master, plus this file's 2 passing and 2 xfailed).
  • ruff clean on the test file; my added source lines have no new findings.

Not verified

  • Only one scenario (sparse dome, FDI grid). Other interpolators (PLI, P2), inequality pairs, and the constant-norm iterations path were not exercised against the fix, beyond the existing tests passing.
  • The native solver path does not use this code.
  • Whether the same unscaled-Q issue exists anywhere else scaling_matrix is used.

🤖 Generated with Claude Code

lachlangrose and others added 2 commits October 29, 2026 14:50
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>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant