-
Notifications
You must be signed in to change notification settings - Fork 214
Fix Ruiz equilibration skip heuristic to also check column imbalance #1601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ed675a1
d43e238
21938ff
5ea14b8
92e20ef
a6bcbfa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -217,4 +217,56 @@ TEST(barrier, min_x_squared_free_variable_dual_correction) | |||||||||||||||||||||
| EXPECT_NEAR(h_z[0], 0.0, tol); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| TEST(barrier, qplib_8515_column_imbalance) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| // Regression test for the Ruiz equilibration skip heuristic in scaling(). | ||||||||||||||||||||||
| // QPLIB_8515 has balanced rows (max-entry ratio 2) but severely imbalanced | ||||||||||||||||||||||
| // columns: its free variables appear in the constraint matrix only with | ||||||||||||||||||||||
| // ~1e-8 coefficients against O(1) rows. | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // Reference objective 319.9999 from https://qplib.zib.de/QPLIB_8515.html. | ||||||||||||||||||||||
| const raft::handle_t handle{}; | ||||||||||||||||||||||
| init_handler(&handle); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| auto path = | ||||||||||||||||||||||
| cuopt::test::get_rapids_dataset_root_dir() + "/quadratic_programming/qplib/QPLIB_8515.lp"; | ||||||||||||||||||||||
| auto mps_data = io::read_lp<int, double>(path); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| auto settings = pdlp_solver_settings_t<int, double>{}; | ||||||||||||||||||||||
| settings.method = method_t::Barrier; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| auto solution = solve_lp(&handle, mps_data, settings); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| EXPECT_EQ(solution.get_termination_status(), pdlp_termination_status_t::Optimal); | ||||||||||||||||||||||
| EXPECT_NEAR(solution.get_objective_value(), 319.9999, 1e-2); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // With equilibration (the default), the well-conditioned barrier converges | ||||||||||||||||||||||
| // quickly (~14 iterations); un-equilibrated it needs far more. | ||||||||||||||||||||||
| const int iters = solution.get_additional_termination_information().number_of_steps_taken; | ||||||||||||||||||||||
| EXPECT_LT(iters, 30); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| TEST(barrier, qplib_8515_ruiz_forced_off) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| // Forcing Ruiz equilibration off (=0) leaves QPLIB_8515's severe column | ||||||||||||||||||||||
| // imbalance in place, so the barrier needs far more iterations than the ~14 | ||||||||||||||||||||||
| // it takes when equilibrated. The blow-up proves scaling() consumes | ||||||||||||||||||||||
| // qcqp_hyper_ruiz_equilibration. | ||||||||||||||||||||||
| const raft::handle_t handle{}; | ||||||||||||||||||||||
| init_handler(&handle); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| auto path = | ||||||||||||||||||||||
| cuopt::test::get_rapids_dataset_root_dir() + "/quadratic_programming/qplib/QPLIB_8515.lp"; | ||||||||||||||||||||||
| auto mps_data = io::read_lp<int, double>(path); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| auto settings = pdlp_solver_settings_t<int, double>{}; | ||||||||||||||||||||||
| settings.method = method_t::Barrier; | ||||||||||||||||||||||
| settings.qcqp_ruiz_equilibration = 0; // force off | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| auto solution = solve_lp(&handle, mps_data, settings); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const int iters = solution.get_additional_termination_information().number_of_steps_taken; | ||||||||||||||||||||||
| EXPECT_GT(iters, 50); | ||||||||||||||||||||||
|
Comment on lines
+266
to
+269
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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 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
Suggested change
🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| } // namespace cuopt::mathematical_optimization::simplex::test | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| #!/bin/bash | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| INSTANCES=( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we only downloading this specific example?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's the first instance from qplib that we're using in tests. |
||
| "QPLIB_8515" | ||
| ) | ||
|
|
||
| BASE_URL="https://qplib.zib.de/lp" | ||
| BASEDIR=$(dirname "$0") | ||
| OUTDIR="${BASEDIR}/qplib" | ||
|
|
||
| mkdir -p "${OUTDIR}" | ||
|
|
||
| ################################################################################ | ||
| # S3 Download Support | ||
| ################################################################################ | ||
| # Requires explicit CUOPT credentials to avoid using unintended AWS credentials: | ||
| # - CUOPT_S3_URI: Base S3 bucket root (e.g., s3://cuopt-datasets/) | ||
| # - CUOPT_AWS_ACCESS_KEY_ID: AWS access key | ||
| # - CUOPT_AWS_SECRET_ACCESS_KEY: AWS secret key | ||
| # - CUOPT_AWS_REGION (optional): AWS region, defaults to us-east-1 | ||
|
|
||
| function try_download_from_s3() { | ||
| if [ -z "${CUOPT_S3_URI:-}" ]; then | ||
| echo "WARNING: CUOPT_S3_URI not set — S3 dataset download disabled, using HTTP fallback." >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| # Require explicit CUOPT credentials to avoid accidentally using generic AWS credentials | ||
| if [ -z "${CUOPT_AWS_ACCESS_KEY_ID:-}" ]; then | ||
| echo "WARNING: CUOPT_AWS_ACCESS_KEY_ID not set — cannot download datasets from S3." >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| if [ -z "${CUOPT_AWS_SECRET_ACCESS_KEY:-}" ]; then | ||
| echo "WARNING: CUOPT_AWS_SECRET_ACCESS_KEY not set — cannot download datasets from S3." >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| if ! command -v aws &> /dev/null; then | ||
| echo "WARNING: AWS CLI not found — cannot download datasets from S3." >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| # Append ci_datasets/quadratic_programming/qplib subdirectory to base S3 URI | ||
| local s3_uri="${CUOPT_S3_URI}ci_datasets/quadratic_programming/qplib/" | ||
| echo "Downloading QPLIB datasets from S3..." | ||
|
|
||
| # Use CUOPT-specific credentials only | ||
| local region="${CUOPT_AWS_REGION:-us-east-1}" | ||
|
|
||
| # Export credentials for AWS CLI | ||
| export AWS_ACCESS_KEY_ID="$CUOPT_AWS_ACCESS_KEY_ID" | ||
| export AWS_SECRET_ACCESS_KEY="$CUOPT_AWS_SECRET_ACCESS_KEY" | ||
| # Unset session token to avoid mixing credentials | ||
| unset AWS_SESSION_TOKEN | ||
| export AWS_DEFAULT_REGION="$region" | ||
|
|
||
| # Test AWS credentials | ||
| if ! aws sts get-caller-identity &> /dev/null 2>&1; then | ||
| echo "AWS credentials invalid, skipping S3 download..." | ||
| return 1 | ||
| fi | ||
|
|
||
| local success=true | ||
| local total=${#INSTANCES[@]} | ||
| local count=0 | ||
| for instance in "${INSTANCES[@]}"; do | ||
| count=$((count + 1)) | ||
| if ! aws s3 cp "${s3_uri}${instance}.lp" "${OUTDIR}/${instance}.lp" --only-show-errors; then | ||
| success=false | ||
| fi | ||
| printf "\rProgress: %d/%d" "$count" "$total" | ||
| done | ||
| echo "" | ||
|
|
||
| if $success; then | ||
| echo "✓ Downloaded QPLIB datasets from S3" | ||
| return 0 | ||
| else | ||
| echo "S3 download failed, falling back to HTTP..." | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| # Try S3 first | ||
| if try_download_from_s3; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # HTTP fallback | ||
| echo "Downloading QPLIB datasets from HTTP..." | ||
| for INSTANCE in "${INSTANCES[@]}"; do | ||
| URL="${BASE_URL}/${INSTANCE}.lp" | ||
| OUTFILE="${OUTDIR}/${INSTANCE}.lp" | ||
|
|
||
| wget -4 --tries=3 --continue --progress=dot:mega --retry-connrefused "${URL}" -O "${OUTFILE}" || { | ||
| echo "Failed to download: ${URL}" | ||
| continue | ||
| } | ||
| done | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is correct because we want to ignore the downloaded instances, while not ignoring the checked-in instances.