Scale the variables with variable bounds - #1652
Conversation
📝 WalkthroughWalkthroughThe benchmark scan now includes compressed MPS and SIF files. SOCP/QP scaling now applies bound-based column pre-scaling before Ruiz equilibration and updates associated problem data. ChangesCompressed model discovery
QP/SOCP bound-based scaling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/dual_simplex/scaling.cpp (1)
89-108: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftBound-based pre-scaling is placed after the early
skip_ruizreturn, so it never runs for the scenario this PR targets.The new block at lines 118-189 sits after the
skip_ruizcheck at lines 89-108.skip_ruizbecomestrue(in automatic mode,ruiz_mode < 0) wheneverrow_norm_ratio,col_norm_ratio, andq_ratioare all small — i.e., whenever the coefficient matrix is already balanced. The new block's own comment states its purpose is to fix exactly this case: a constraint matrix that is "already be perfectly balanced (all +/-1) while the variables themselves live at wildly different scales." When that case occurs,skip_ruizistrue, the function returns at line 107, and the bound-based pre-scaling at lines 118-189 never executes. The feature is unreachable for its primary target scenario.Move the bound-magnitude pre-scaling block to run unconditionally, before the row/column-norm-ratio computation used for the
skip_ruizdecision (i.e., right aftercol_scaleis initialized at line 37). This also lets the ratio computation reflect the post-prescaling matrix, which is consistent with the intent described in the new block's comment.If you move the block earlier, also update the
skip_ruizbranch:column_scaling.assign(n, 1.0)at line 106 must instead reflect the bound-prescaling that has already been applied toscaled.A,scaled.objective, andscaled.lower/scaled.upper(i.e.,column_scaling[j] = 1 / col_scale[j]), otherwise the returnedcolumn_scalingwill not match the actually-scaled problem andunscale_solutionwill produce wrong results downstream.🐛 Proposed structural fix (schematic)
if (!unscaled.second_order_cone_dims.empty() || unscaled.Q.n > 0) { // col_scale and row_scale accumulate reciprocal scale factors during Ruiz iterations. std::vector<f_t> col_scale(n, 1.0); + // Bound-magnitude column pre-scaling must run before the imbalance + // heuristic below: it targets exactly the case where A/Q are already + // balanced but variable bounds are not. + { + // ... (existing block body, currently at lines 136-188, unchanged) ... + } + // Decide whether Ruiz scaling is needed by checking row- and column-norm // imbalance. ... csr_matrix_t<i_t, f_t> Arow_check(0, 0, 0); scaled.A.to_compressed_row(Arow_check); ... if (skip_ruiz) { ... - column_scaling.assign(n, 1.0); + column_scaling.resize(n); + for (i_t j = 0; j < n; ++j) column_scaling[j] = f_t(1) / col_scale[j]; return 0; } ... - // ----------------------------------------------------------------------- - // Bound-magnitude column pre-scaling. - // ----------------------------------------------------------------------- - { ... } // remove from this locationAlso applies to: 118-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/scaling.cpp` around lines 89 - 108, Move the bound-magnitude pre-scaling block in the scaling routine to immediately after col_scale initialization, before row_norm_ratio, col_norm_ratio, and q_ratio are computed, so it runs even when automatic skip_ruiz would otherwise return early and the ratios use the prescaled matrix. In the skip_ruiz branch, replace the identity column_scaling assignment with values derived from the applied prescaling, setting each entry to the reciprocal of col_scale so unscale_solution matches scaled.A, scaled.objective, and scaled.lower/upper.
🧹 Nitpick comments (1)
cpp/src/dual_simplex/scaling.cpp (1)
118-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit test coverage for the new bound-based pre-scaling logic.
This block introduces new numeric logic (geometric-mean bound scaling, propagation into
A,objective, bounds, andQ). Two correctness bugs were found by inspection alone, which shows this logic needs direct test coverage. Add GoogleTest cases undercpp/src/testsexercisingscaling()with variable bounds spanning several orders of magnitude (including free variables, one-sided bounds, and cone variables), and assert that scaled bounds become O(1) and thatcolumn_scalingcorrectly reconstructs the original bounds viaunscale_solution.Run the following to check whether coverage already exists:
Based on coding guidelines, "Contributors must add unit tests for code changes, using GoogleTest examples under
cpp/src/testsfor C/C++."#!/bin/bash # Description: Check for existing test coverage of bound-based column pre-scaling in dual_simplex scaling. fd . cpp/src/tests --type f -e cpp -e hpp | xargs rg -n -C3 -i 'scaling|c0|geo_mean|bound.*scal'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/scaling.cpp` around lines 118 - 189, Add GoogleTest coverage under cpp/src/tests for the scaling() bound-based pre-scaling path, using variables with bounds spanning several orders of magnitude and including free, one-sided, and cone variables. Assert that finite scaled bounds are O(1), and verify column_scaling reconstructs the original bounds through unscale_solution. Reuse existing scaling test fixtures and conventions, and cover propagation through A, objective, and Q where supported.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/linear_programming/run_mps_files.sh`:
- Line 363: Update the input-name normalization in the benchmark
artifact-generation logic, including the relevant naming code in run_mip.cpp, to
remove a trailing .gz first and then strip .mps, .MPS, or .SIF suffixes
case-consistently. Ensure all newly supported compressed and uppercase
extensions produce artifact names without the source suffix.
In `@cpp/src/dual_simplex/scaling.cpp`:
- Around line 136-166: Track which columns receive a finite-bound magnitude in
the scaling setup, using a per-column mask alongside c0. In the normalization
loop associated with geo_mean, divide only masked columns by geo_mean so columns
skipped by the first loop remain exactly 1.0 and continue to bypass later
scaling.
---
Outside diff comments:
In `@cpp/src/dual_simplex/scaling.cpp`:
- Around line 89-108: Move the bound-magnitude pre-scaling block in the scaling
routine to immediately after col_scale initialization, before row_norm_ratio,
col_norm_ratio, and q_ratio are computed, so it runs even when automatic
skip_ruiz would otherwise return early and the ratios use the prescaled matrix.
In the skip_ruiz branch, replace the identity column_scaling assignment with
values derived from the applied prescaling, setting each entry to the reciprocal
of col_scale so unscale_solution matches scaled.A, scaled.objective, and
scaled.lower/upper.
---
Nitpick comments:
In `@cpp/src/dual_simplex/scaling.cpp`:
- Around line 118-189: Add GoogleTest coverage under cpp/src/tests for the
scaling() bound-based pre-scaling path, using variables with bounds spanning
several orders of magnitude and including free, one-sided, and cone variables.
Assert that finite scaled bounds are O(1), and verify column_scaling
reconstructs the original bounds through unscale_solution. Reuse existing
scaling test fixtures and conventions, and cover propagation through A,
objective, and Q where supported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9c08735c-434e-462e-9f51-0916d37184ba
📒 Files selected for processing (2)
benchmarks/linear_programming/run_mps_files.shcpp/src/dual_simplex/scaling.cpp
| else | ||
| # Gather .mps/.MPS and .SIF files in the directory | ||
| mapfile -t mps_files < <(ls "$MPS_DIR"/*.mps "$MPS_DIR"/*.MPS "$MPS_DIR"/*.SIF 2>/dev/null) | ||
| mapfile -t mps_files < <(ls "$MPS_DIR"/*.mps "$MPS_DIR"/*.MPS "$MPS_DIR"/*.SIF "$MPS_DIR"/*.mps.gz "$MPS_DIR"/*.MPS.gz "$MPS_DIR"/*.SIF.gz 2>/dev/null) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Normalize names for all newly supported extensions.
Line 363 adds .MPS.gz and .SIF.gz, but benchmarks/linear_programming/cuopt/run_mip.cpp strips only lowercase .mps when it creates log names. For these new inputs, the lookup returns npos, so names retain the compressed suffix, such as model.SIF.gz.log. Normalize .gz first, then strip .mps, .MPS, or .SIF consistently before generating benchmark artifacts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/linear_programming/run_mps_files.sh` at line 363, Update the
input-name normalization in the benchmark artifact-generation logic, including
the relevant naming code in run_mip.cpp, to remove a trailing .gz first and then
strip .mps, .MPS, or .SIF suffixes case-consistently. Ensure all newly supported
compressed and uppercase extensions produce artifact names without the source
suffix.
| std::vector<f_t> c0(n, 1.0); | ||
| const i_t cone_start0 = | ||
| unscaled.second_order_cone_dims.empty() ? n : unscaled.cone_var_start; | ||
| f_t geo_sum = 0.0; | ||
| i_t geo_count = 0; | ||
| for (i_t j = 0; j < cone_start0; ++j) { | ||
| f_t lo = std::abs(scaled.lower[j]); | ||
| f_t hi = std::abs(scaled.upper[j]); | ||
| f_t mag; | ||
| bool lo_fin = scaled.lower[j] > -1e20 && lo > 0; | ||
| bool hi_fin = scaled.upper[j] < 1e20 && hi > 0; | ||
| if (lo_fin && hi_fin) { | ||
| mag = std::sqrt(lo * hi); | ||
| } else if (lo_fin) { | ||
| mag = lo; | ||
| } else if (hi_fin) { | ||
| mag = hi; | ||
| } else { | ||
| continue; // free / one-sided-zero: leave at scale 1 | ||
| } | ||
| c0[j] = mag; | ||
| geo_sum += std::log(mag); | ||
| geo_count++; | ||
| } | ||
| if (geo_count > 0) { | ||
| // Normalize so the average column scale is 1, keeping the overall | ||
| // problem magnitude centered rather than uniformly shrinking it. | ||
| const f_t geo_mean = std::exp(geo_sum / static_cast<f_t>(geo_count)); | ||
| for (i_t j = 0; j < cone_start0; ++j) { | ||
| c0[j] /= geo_mean; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Normalization loop rescales columns that have no finite bound, contradicting the "leave at scale 1" intent.
The comment at line 134 states: "Columns with no finite, nonzero bound are left at scale 1." The first loop (lines 141-159) honors that by continue-ing past free columns, leaving their c0[j] at the default 1.0. But the normalization loop at lines 164-166 divides every c0[j] in [0, cone_start0) by geo_mean, including the untouched default-1.0 entries. This turns free columns' c0[j] into 1.0 / geo_mean, which then fails the if (c0[j] == 1.0) continue; check at line 170 and causes scaled.A, scaled.objective, scaled.lower/scaled.upper, and scaled.Q entries for free variables to be scaled incorrectly.
Track which columns actually received a finite-bound magnitude and only normalize those.
🐛 Proposed fix using an explicit "has bound" mask
std::vector<f_t> c0(n, 1.0);
const i_t cone_start0 =
unscaled.second_order_cone_dims.empty() ? n : unscaled.cone_var_start;
f_t geo_sum = 0.0;
i_t geo_count = 0;
+ std::vector<bool> has_bound(n, false);
for (i_t j = 0; j < cone_start0; ++j) {
f_t lo = std::abs(scaled.lower[j]);
f_t hi = std::abs(scaled.upper[j]);
f_t mag;
bool lo_fin = scaled.lower[j] > -1e20 && lo > 0;
bool hi_fin = scaled.upper[j] < 1e20 && hi > 0;
if (lo_fin && hi_fin) {
mag = std::sqrt(lo * hi);
} else if (lo_fin) {
mag = lo;
} else if (hi_fin) {
mag = hi;
} else {
continue; // free / one-sided-zero: leave at scale 1
}
c0[j] = mag;
+ has_bound[j] = true;
geo_sum += std::log(mag);
geo_count++;
}
if (geo_count > 0) {
const f_t geo_mean = std::exp(geo_sum / static_cast<f_t>(geo_count));
for (i_t j = 0; j < cone_start0; ++j) {
- c0[j] /= geo_mean;
+ if (has_bound[j]) c0[j] /= geo_mean;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::vector<f_t> c0(n, 1.0); | |
| const i_t cone_start0 = | |
| unscaled.second_order_cone_dims.empty() ? n : unscaled.cone_var_start; | |
| f_t geo_sum = 0.0; | |
| i_t geo_count = 0; | |
| for (i_t j = 0; j < cone_start0; ++j) { | |
| f_t lo = std::abs(scaled.lower[j]); | |
| f_t hi = std::abs(scaled.upper[j]); | |
| f_t mag; | |
| bool lo_fin = scaled.lower[j] > -1e20 && lo > 0; | |
| bool hi_fin = scaled.upper[j] < 1e20 && hi > 0; | |
| if (lo_fin && hi_fin) { | |
| mag = std::sqrt(lo * hi); | |
| } else if (lo_fin) { | |
| mag = lo; | |
| } else if (hi_fin) { | |
| mag = hi; | |
| } else { | |
| continue; // free / one-sided-zero: leave at scale 1 | |
| } | |
| c0[j] = mag; | |
| geo_sum += std::log(mag); | |
| geo_count++; | |
| } | |
| if (geo_count > 0) { | |
| // Normalize so the average column scale is 1, keeping the overall | |
| // problem magnitude centered rather than uniformly shrinking it. | |
| const f_t geo_mean = std::exp(geo_sum / static_cast<f_t>(geo_count)); | |
| for (i_t j = 0; j < cone_start0; ++j) { | |
| c0[j] /= geo_mean; | |
| } | |
| std::vector<f_t> c0(n, 1.0); | |
| const i_t cone_start0 = | |
| unscaled.second_order_cone_dims.empty() ? n : unscaled.cone_var_start; | |
| f_t geo_sum = 0.0; | |
| i_t geo_count = 0; | |
| std::vector<bool> has_bound(n, false); | |
| for (i_t j = 0; j < cone_start0; ++j) { | |
| f_t lo = std::abs(scaled.lower[j]); | |
| f_t hi = std::abs(scaled.upper[j]); | |
| f_t mag; | |
| bool lo_fin = scaled.lower[j] > -1e20 && lo > 0; | |
| bool hi_fin = scaled.upper[j] < 1e20 && hi > 0; | |
| if (lo_fin && hi_fin) { | |
| mag = std::sqrt(lo * hi); | |
| } else if (lo_fin) { | |
| mag = lo; | |
| } else if (hi_fin) { | |
| mag = hi; | |
| } else { | |
| continue; // free / one-sided-zero: leave at scale 1 | |
| } | |
| c0[j] = mag; | |
| has_bound[j] = true; | |
| geo_sum += std::log(mag); | |
| geo_count++; | |
| } | |
| if (geo_count > 0) { | |
| // Normalize so the average column scale is 1, keeping the overall | |
| // problem magnitude centered rather than uniformly shrinking it. | |
| const f_t geo_mean = std::exp(geo_sum / static_cast<f_t>(geo_count)); | |
| for (i_t j = 0; j < cone_start0; ++j) { | |
| if (has_bound[j]) c0[j] /= geo_mean; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/dual_simplex/scaling.cpp` around lines 136 - 166, Track which columns
receive a finite-bound magnitude in the scaling setup, using a per-column mask
alongside c0. In the normalization loop associated with geo_mean, divide only
masked columns by geo_mean so columns skipped by the first loop remain exactly
1.0 and continue to bypass later scaling.
|
In the past I've seen some benchmark problems with very large, artificial bounds that don't correspond to the appropriate magnitude of the variables. I'm a bit concerned that such large bounds could have a noisy or negative impact on the solve if we rescale like this. |
- Scale columns so variables become O(1) using geometric mean of finite bounds - Prevents ill-conditioning when variable bounds span many orders of magnitude - Particularly helps network-flow QPs where bounds range from 1e4 to 1e11 - Applied before Ruiz equilibration, which then balances coefficient magnitudes
Benchmarks show regressions on QP though there are some good wins on LP benchmarks. I think its not worth pushing this change. |
2 participants
Description
Pre-scale columns by variable bound magnitude before Ruiz equilibration to improve conditioning when variable bounds span many orders of magnitude.
Changes:
Motivation:
Some QP problems (particularly network-flow formulations) have variable bounds ranging from O(1) to O(10^11). Without pre-scaling, the barrier solver sees variables at vastly different scales, leading to ill-conditioned KKT systems. By normalizing variables to O(1) before Ruiz equilibration, we improve numerical stability.
Benchmark Results:
Acknowledgment:
This improvement was proposed by the Hiverge AI discovery engine with experiments by @kerry-hiverge.
Issue
Checklist