Fix Ruiz equilibration skip heuristic to also check column imbalance#1601
Fix Ruiz equilibration skip heuristic to also check column imbalance#1601mlubin wants to merge 6 commits into
Conversation
b38e814 to
8ba9578
Compare
The skip heuristic in scaling() only measured row-norm imbalance, so a QP/SOCP with balanced rows but severely imbalanced columns (e.g. QPLIB_8515, where free variables appear only with ~1e-8 coefficients against O(1) rows) received no scaling at all. The resulting augmented KKT system was so ill-conditioned that barrier terminated at a suboptimal point while reporting convergence (320.13 vs the true optimum 320.00 on QPLIB_8515, non-deterministically across runs). With the column check, Ruiz runs and the instance solves to 319.999989 in 14 iterations instead of 338. Adds QPLIB_8515 as a downloaded test dataset (new download_qplib_test_dataset.sh, wired into ci/test_cpp.sh and CONTRIBUTING.md) and a full-solve regression test that asserts the optimal objective matches the QPLIB reference value. Signed-off-by: Miles Lubin <mlubin@nvidia.com>
8ba9578 to
ed675a1
Compare
mlubin
added
bug
|
/ok to test ed675a1 |
CI Test Summary✅ All 0 test job(s) passed. |
| # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| INSTANCES=( |
There was a problem hiding this comment.
Are we only downloading this specific example?
There was a problem hiding this comment.
Yes, it's the first instance from qplib that we're using in tests.
| } | ||
| f_t col_norm_ratio = (min_col_norm > 0) ? max_col_norm / min_col_norm : 1.0; | ||
|
|
||
| if (row_norm_ratio < 100.0 && col_norm_ratio < 100.0) { |
There was a problem hiding this comment.
We might want to add an option --row-scaling that by default is -1 and we use the heuristic below, but if the user sets --row-scaling=0 they can turn it off, and if they set --row-scaling=1 they can force it on.
The skip heuristic now checks both row-norm and column-norm imbalance, but the comment still described only row-norm imbalance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Miles Lubin <mlubin@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe QPLIB downloader now supports S3 with HTTP fallback and is wired into CI and contributor instructions. QCQP Ruiz equilibration is configurable, and barrier scaling uses both row and column imbalance ratios with QPLIB regression tests. QPLIB scaling regression
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🤖 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 `@datasets/quadratic_programming/download_qplib_test_dataset.sh`:
- Line 1: Update the dataset download script to enable strict shell error
handling and ensure a failed wget for QPLIB_8515.lp exits with a nonzero status
instead of being masked by continue.
🪄 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: 4a58299f-8732-465d-aa64-49ef9aeee586
📒 Files selected for processing (6)
.gitignoreCONTRIBUTING.mdci/test_cpp.shcpp/src/dual_simplex/scaling.cppcpp/tests/dual_simplex/unit_tests/solve_barrier.cudatasets/quadratic_programming/download_qplib_test_dataset.sh
ramakrishnap-nv
left a comment
ramakrishnap-nv
left a comment
There was a problem hiding this comment.
A minor suggestion
| !datasets/quadratic_programming | ||
| !datasets/quadratic_programming/** | ||
| # downloaded QPLIB instances (see download_qplib_test_dataset.sh) | ||
| datasets/quadratic_programming/qplib/ |
There was a problem hiding this comment.
| datasets/quadratic_programming/qplib/ | |
| !datasets/quadratic_programming/qplib/ |
Should this be
There was a problem hiding this comment.
I think this is correct because we want to ignore the downloaded instances, while not ignoring the checked-in instances.
rg20
left a comment
rg20
left a comment
There was a problem hiding this comment.
Thanks for the improvements!
|
/ok to test d43e238 |
Signed-off-by: Miles Lubin <mlubin@nvidia.com>
Signed-off-by: Miles Lubin <mlubin@nvidia.com>
Drop the redundant force-on solve from the toggle test (it re-treads the equilibrated path); force-off now asserts a >50-iteration blow-up, and the default-path test bounds the equilibrated solve to <30 iterations. Remove the redundant Python registry test (already covered by test_solver_settings) and trim the redundant "hidden from --help" note from the QCQP comments. Signed-off-by: Miles Lubin <mlubin@nvidia.com>
|
/ok to test 92e20ef |
The tri-state switch in scaling() was hand-aligned in a way clang-format rejects; reformat to satisfy ci/check_style.sh. Signed-off-by: Miles Lubin <mlubin@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cpp/tests/dual_simplex/unit_tests/solve_barrier.cu`:
- Around line 266-269: Update the test around solve_lp and the existing solution
object to first assert optimal termination and verify the objective matches the
319.9999 reference value, then retain the iteration-count assertion. Ensure the
numerical correctness checks occur before validating that more than 50 steps
were taken.
🪄 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: 6e24c9fd-4b16-4a8e-86ba-67ccfdf64b5d
📒 Files selected for processing (3)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/src/math_optimization/solver_settings.cucpp/tests/dual_simplex/unit_tests/solve_barrier.cu
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/include/cuopt/mathematical_optimization/constants.h
- cpp/src/math_optimization/solver_settings.cu
| auto solution = solve_lp(&handle, mps_data, settings); | ||
|
|
||
| const int iters = solution.get_additional_termination_information().number_of_steps_taken; | ||
| EXPECT_GT(iters, 50); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require a successful solve before asserting the iteration blow-up.
This test currently passes for any result taking more than 50 steps, including a non-optimal or iteration-limited termination. Assert optimal termination and the 319.9999 reference objective before checking the iteration count.
As per path instructions, regression tests must validate numerical correctness, not only iteration behavior.
Proposed fix
auto solution = solve_lp(&handle, mps_data, settings);
+ ASSERT_EQ(solution.get_termination_status(), pdlp_termination_status_t::Optimal);
+ EXPECT_NEAR(solution.get_objective_value(), 319.9999, 1e-2);
const int iters = solution.get_additional_termination_information().number_of_steps_taken;
EXPECT_GT(iters, 50);📝 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.
| auto solution = solve_lp(&handle, mps_data, settings); | |
| const int iters = solution.get_additional_termination_information().number_of_steps_taken; | |
| EXPECT_GT(iters, 50); | |
| auto solution = solve_lp(&handle, mps_data, settings); | |
| ASSERT_EQ(solution.get_termination_status(), pdlp_termination_status_t::Optimal); | |
| EXPECT_NEAR(solution.get_objective_value(), 319.9999, 1e-2); | |
| const int iters = solution.get_additional_termination_information().number_of_steps_taken; | |
| EXPECT_GT(iters, 50); |
🤖 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/tests/dual_simplex/unit_tests/solve_barrier.cu` around lines 266 - 269,
Update the test around solve_lp and the existing solution object to first assert
optimal termination and verify the objective matches the 319.9999 reference
value, then retain the iteration-count assertion. Ensure the numerical
correctness checks occur before validating that more than 50 steps were taken.
Source: Path instructions
|
/ok to test a6bcbfa |
5 participants
The skip heuristic in scaling() only measured row-norm imbalance, so a QP/SOCP with balanced rows but severely imbalanced columns (e.g. QPLIB_8515, where free variables appear only with ~1e-8 coefficients against O(1) rows) received no scaling at all. The resulting augmented KKT system was so ill-conditioned that barrier terminated at a suboptimal point while reporting convergence (320.13 vs the true optimum 320.00 on QPLIB_8515, non-deterministically across runs). With the column check, Ruiz runs and the instance solves to 319.999989 in 14 iterations instead of 338.
Adds QPLIB_8515 as a downloaded test dataset (new
download_qplib_test_dataset.sh, wired into ci/test_cpp.sh and CONTRIBUTING.md) and a full-solve regression test that asserts the optimal objective matches the QPLIB reference value.
A summary of impacts on public benchmark instances that are affected:
Improvements
Neutral
Regressions
Adds a
--qcqp_hyper_ruiz_equilibrationflag to override the behavior.