Fix issue with infinite lower bounds and try to bound free variables in barrier#1001
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a barrier presolve pass that tightens/removes certain free variables and negates variables with only an upper bound (x' = -x), updates objective/A/Q accordingly, records negated indices and undoes them in solution uncrushing, introduces CLI file-read timing/logging, and adjusts barrier residual/objective logging and normalization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 the current code and only fix it if needed.
Inline comments:
In `@cpp/src/dual_simplex/presolve.cpp`:
- Around line 926-944: Guard the divisions by a_ij in the bounding logic by
skipping or avoiding division when |a_ij| is near zero: introduce a small
tolerance (e.g. tol) and before any 1.0 / a_ij operations check if
std::abs(a_ij) > tol; if not, do not update problem.upper[j] or problem.lower[j]
(and do not set bounded) or handle via a safe fallback; apply this check around
the four locations using the same tol and keep the existing conditions involving
lower_inf_i, upper_inf_i, lower_activity_i, upper_activity_i, rhs and bounded so
only numerically safe divisions occur.
🪄 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: Pro
Run ID: de8c026d-5d75-460d-a9a8-fb066f9cc6f1
📒 Files selected for processing (2)
cpp/src/dual_simplex/presolve.cppcpp/src/dual_simplex/presolve.hpp
cpp/src/dual_simplex/presolve.hpp
Outdated
|
|
||
| folding_info_t<i_t, f_t> folding_info; | ||
|
|
||
| std::vector<i_t> new_slacks; |
There was a problem hiding this comment.
Whoops. This line should be removed.
|
@rg20 to review |
|
|
||
| // The constraints are in the form: | ||
| // sum_j a_j x_j = beta | ||
| for (i_t i : constraints_to_check) { |
There was a problem hiding this comment.
I think we can repurpose the bound strengthening code here.
Remove `new_slacks` variable.
|
/ok to test 4432c27 |
…ig. Use user object for relative complementarity gap. Initialize logger so we get feedback when reading large mps files
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/barrier/barrier.cu`:
- Around line 3493-3497: The complementarity normalization uses
compute_user_objective() which includes lp.obj_constant; remove that constant
from the denominator so an arbitrary objective offset cannot shrink the
relative_complementarity_residual. Change the normalization to use the user
objective without lp.obj_constant (either call a new helper like
compute_user_objective_without_constant or subtract lp.obj_constant from
compute_user_objective(lp, primal_objective) before taking std::abs) when
computing relative_complementarity_residual (and the other occurrence noted),
leaving compute_user_objective() behavior unchanged.
🪄 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: Pro
Run ID: 44e4dc72-db04-4343-ab6d-560f43805934
📒 Files selected for processing (3)
cpp/cuopt_cli.cppcpp/src/barrier/barrier.cucpp/src/dual_simplex/presolve.cpp
✅ Files skipped from review due to trivial changes (1)
- cpp/src/dual_simplex/presolve.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/dual_simplex/presolve.cpp (1)
861-904: Verify iterative bound propagation handles partial bounds correctly.The activity computation uses current bounds, which may have been modified by previous constraint iterations. This implements iterative implied bound propagation. However, if a variable receives only one bound (e.g., only upper set at line 931, not lower), it transitions from counting as infinite to contributing finite activity for subsequent constraints.
This is mathematically correct behavior for bound propagation, but consider whether a single-pass approach might miss tightening opportunities compared to iterating until fixpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/dual_simplex/presolve.cpp` around lines 861 - 904, The current single-pass loop over constraints_to_check (inside the block using Arow, problem.lower/problem.upper and variables like lower_activity_i, upper_activity_i, lower_inf_i, upper_inf_i, last_free_i) can miss tightening opportunities because bounds updated earlier in the pass change contribution status for later rows; change the algorithm to iterate until a fixpoint: repeat scanning the relevant constraints (or use a worklist/queue of affected constraints) and recompute lower_activity_i/upper_activity_i for each row whenever a variable bound changes, marking any tightened bound and re-enqueueing its incident constraints, and stop when no bound is tightened in a full pass so all implied bounds are propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/src/dual_simplex/presolve.cpp`:
- Around line 861-904: The current single-pass loop over constraints_to_check
(inside the block using Arow, problem.lower/problem.upper and variables like
lower_activity_i, upper_activity_i, lower_inf_i, upper_inf_i, last_free_i) can
miss tightening opportunities because bounds updated earlier in the pass change
contribution status for later rows; change the algorithm to iterate until a
fixpoint: repeat scanning the relevant constraints (or use a worklist/queue of
affected constraints) and recompute lower_activity_i/upper_activity_i for each
row whenever a variable bound changes, marking any tightened bound and
re-enqueueing its incident constraints, and stop when no bound is tightened in a
full pass so all implied bounds are propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d604ec33-375d-4234-9125-cefe3d8439a7
📒 Files selected for processing (1)
cpp/src/dual_simplex/presolve.cpp
|
/ok to test c7816fa |
|
/ok to test 9133db2 |
|
/ok to test 545dfcf |
|
/ok to test 178f934 |
3 participants
Results in a 1.07X speed up on GH200. Note that
Linf_520chas a big performance variability.