Add a guard for huge bounds in bounds propagation#1390
Open
hlinsen wants to merge 8 commits into
Open
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:
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)
📝 WalkthroughWalkthroughAdds a per-update candidate_bound_scale stored in bounds update data, a device predicate that gates candidate bound writes using a scaled absolute-tolerance check, wires the scale from presolve/multi-probe, and adds solver-setting validation for those tolerances. ChangesBound Update Validation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/mip_heuristics/presolve/bounds_update_helpers.cuh`:
- Around line 46-47: The hardcoded acceptance scale (candidate_bound_scale =
f_t{1e-14}) in bounds_update_helpers.cuh makes bound gating inflexible; change
the code to derive this scale from the solver's configured tolerances (or accept
a passed-in relative tolerance) instead of the magic constant: replace the
literal with a parameter or a value read from the solver settings (e.g.,
solver_tolerances.relative_bound_tol or an added function parameter rel_tol) and
use that derived value in the existing comparison that uses
candidate_bound_scale, while keeping the existing variables bound and abs_tol
intact.
🪄 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: f63805a4-c0d7-438e-9fec-93725704b9f2
📒 Files selected for processing (1)
cpp/src/mip_heuristics/presolve/bounds_update_helpers.cuh
Contributor
Author
|
/ok to test b4ba3b1 |
Contributor
Author
|
/ok to test 4c513aa |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/mip_heuristics/presolve/bounds_presolve.cu`:
- Around line 121-122: The division computing upd.candidate_bound_scale
(pb.tolerances.absolute_tolerance / context.settings.semi_continuous_big_m) must
be guarded against zero or near-zero denominators; modify the code that sets
upd.candidate_bound_scale to first check context.settings.semi_continuous_big_m
> epsilon (use a small constant like 1e-12 or an existing solver epsilon if
available) and if not, set candidate_bound_scale to a safe clamped value (e.g.
absolute_tolerance / epsilon or a predefined max_scale) so the acceptance
predicate does not get disabled by Inf/huge values; update any related comments
and consider validating semi_continuous_big_m at solver initialization as an
alternative.
In `@cpp/src/mip_heuristics/presolve/multi_probe.cu`:
- Around line 144-147: The division for upd_0.candidate_bound_scale and
upd_1.candidate_bound_scale uses context.settings.semi_continuous_big_m without
guarding against zero/near-zero; update the assignment logic (where
candidate_bound_scale is set for upd_0 and upd_1) to first check that
context.settings.semi_continuous_big_m > epsilon (choose a small epsilon, e.g.
1e-12) and only perform pb.tolerances.absolute_tolerance / semi_continuous_big_m
when that condition holds, otherwise set candidate_bound_scale to a safe
fallback or clamp the computed value into a reasonable range (max_scale) to
avoid infinity/huge values that break the acceptance predicate; reference the
variables upd_0, upd_1, candidate_bound_scale, pb.tolerances.absolute_tolerance,
and context.settings.semi_continuous_big_m when making the change.
🪄 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: 7382ad84-9366-4f1e-89c5-9962d8c2e87b
📒 Files selected for processing (5)
cpp/src/mip_heuristics/presolve/bounds_presolve.cucpp/src/mip_heuristics/presolve/bounds_update_data.cucpp/src/mip_heuristics/presolve/bounds_update_data.cuhcpp/src/mip_heuristics/presolve/bounds_update_helpers.cuhcpp/src/mip_heuristics/presolve/multi_probe.cu
✅ Files skipped from review due to trivial changes (1)
- cpp/src/mip_heuristics/presolve/bounds_update_data.cuh
Contributor
Author
|
/ok to test 2d0e086 |
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/src/mip_heuristics/solve.cu`:
- Around line 339-346: The validation currently allows
settings.tolerances.absolute_tolerance == 0 which disables the huge-bound guard;
change the cuopt_expects condition in the existing validation call to require
absolute_tolerance > f_t(0) (strictly positive) instead of >= f_t(0), and update
the error message text in the same cuopt_expects invocation to state that
mip_absolute_tolerance must be strictly positive (finite and > 0) while keeping
the existing checks for settings.semi_continuous_big_m > mip_absolute_tolerance;
this affects the cuopt_expects invocation that references
settings.tolerances.absolute_tolerance and settings.semi_continuous_big_m.
🪄 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: 895b072f-8592-4012-aebf-3feb7b2b610f
📒 Files selected for processing (3)
cpp/src/mip_heuristics/presolve/bounds_presolve.cucpp/src/mip_heuristics/presolve/multi_probe.cucpp/src/mip_heuristics/solve.cu
Contributor
Author
|
/ok to test 94dbc1a |
Contributor
Author
|
/ok to test 65f9e35 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Semi-continuous var with big-M reformulations can create large row activities where the true residual is
smaller than double precision at that scale.
In that case, floating point cancellation can produce an unsafe candidate bound and later false infeasibility.
We add an usual guard on huge bounds derived. Note that it does not protect against the float cancellation error.
A full cancellation-aware guard would require tracking row activity scale or using compensated activity arithmetic, which would add extra reductions/storage or broader propagation changes.