-
Notifications
You must be signed in to change notification settings - Fork 275
Strong-branching refactor #2444
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## latest #2444 +/- ##
==========================================
+ Coverage 79.64% 79.66% +0.02%
==========================================
Files 346 346
Lines 86490 86356 -134
==========================================
- Hits 68884 68797 -87
+ Misses 17606 17559 -47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jajhall
left a comment
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.
Fine by me. One more minor modification that can be accumulated before baseline testing to assess the effectiveness of expected performance enhancements sitting in PRs
BenChampion
left a comment
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.
Nice! Always in favor of removing unnecessary duplicated code. I'm not sure what the overhead of the lambda would be, but my intuition (possibly wrong!) is that it should be minimal. Worth testing, of course.
|
I ran 424 instances from MIPLIB with these changes and observed different node counts on bell3a, bell5, blp-ar98, harp2, lectsched-4-obj, leo1, lseu, map18, mine-166-5, neos-1440460, neos-476283, noswot, opm2-z7s2, pg, pg5_34, pp08a, ran14x18-disj-8, ran16x16, reblock166, reblock67, rentacar, roll3000 and rout. I am not sure if all these are affected by the single line change you described. |
|
@fwesselm I have just simply removed that line (Thought about it a bit more and there could be some fringe case where such a change slows down an instance by a fair bit) |
Ok, I will re-run the instances where the behavior changed shortly. |
I re-ran the instances, and there are no behavior changes with the updated code. |
|
I tried to resolve the merge conflict and hope this is ready to be merged now. |
|
Assuming the merge conflict was just for the README? I'm all for this being merged. |
Yes, FEATURES.md had a conflict. |
This creates a lambda expression
strongBranchthat gets called when evaluating branching candidates. It is purely there to improve code readability and remove some duplicate code. Previously, there was a large amount of extended logic forx <= band that was mostly repeated forx >= b+1. I've put them into a single function where you give a boolean flag on which one you want to evaluate. That should not change change the solution path whatsoever (completely unsure if there is a potential performance impact for semi-large lambda expressions in C++)The single line change that may change the solution path of some instances: If there is a single branching candidate then I just now select it (I don't bother evaluating it).