Repository navigation
Keep small terms big domains lp aggregator - #3507
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## latest #3507 +/- ##
==========================================
+ Coverage 75.46% 75.50% +0.03%
==========================================
Files 461 461
Lines 113232 113254 +22
Branches 17881 17883 +2
==========================================
+ Hits 85456 85514 +58
+ Misses 27488 27452 -36
Partials 288 288 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fwesselm
left a comment
There was a problem hiding this comment.
Looks good, @Opt-Mucca!
Could termIsZero live in util/HighsUtils.h maybe? I think its usage will grow... :)
It seems a bit closely tied to |
…-small-terms-big-domains-lp-aggregator
@jajhall @Opt-Mucca: I moved the function and added a test. |
|
@fwesselm you beat me to it! Luckily I had just started working on it, so no time wasted. (Been sidetracked by some presolve stuff all day) I like it living in |
Description
See #3478 for a full description of the issue. Essentially: If we drop terms with small coefficients from the LP aggregation without considering their domains then we run the risk of generating invalid cuts. We already made changes along this line for conflict analysis a month or so ago.
This change affects 1/3 instances and at least on my test set is 1% performance negative. I'd say this is a necessary evil, but it still sucks. Interestingly, after the change I am solving more instances and getting more consistent speed-ups than slow-downs across seeds, so this is a nice change for future benchmarking. The big losers that drag down the average seem to be problems with large column domains, where two instances in particular get consistently 2-4x slower. @fwesselm I'm happy for this to wait for further testing.
Checklist
latestbranchRelated issue
Closes #3478