Fix clique size and numerical issue bugs#1589
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesClique graph updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/conflict_graph/clique_table.cu`:
- Around line 33-34: The additional-clique path must use the same strict
tolerance boundary as the initial check: update the logic around the lower_bound
handling to reject candidate sums equal to rhs_with_tolerance and proceed only
when the sum is strictly greater. Add a regression test that includes a valid
base clique and reaches the additional-clique path, verifying equality at the
tolerance boundary does not reintroduce the conflict.
🪄 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: 470cbd38-3ffb-4813-97a8-be8db5576eb4
📒 Files selected for processing (3)
cpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cucpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cuhcpp/tests/mip/cuts_test.cu
aliceb-nv
left a comment
There was a problem hiding this comment.
Good forward thinking for the int64_t for offsets :) We should probably go over the whole codebase someday to ensure all CSRs use 64bit for the offsets. Perhaps a off_t typedef or such and agentic checks
CI Test Summary✅ All 31 test job(s) passed. |
|
/merge |
Small cliques are reduced to pairwise conflicts in adj list format and the limit was 512. For some problems, this is too agressive and creates large amount of adj list entries that exceed int32 size. This PR fixes it by reducing the adj list storage threshold to 64.
This PR also fixes a roundoff error that is revealed after extraction of cliques from mixed rows were instroduced. We check with tolerance now.