-
Notifications
You must be signed in to change notification settings - Fork 112
New heuristic improvements #178
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
New heuristic improvements #178
Conversation
|
/ok to test 1fb206f |
|
/ok to test 1fb206f |
|
/ok to test b61f302 |
|
/ok to test 4a3a0b5 |
|
/ok to test f057b04 |
aliceb-nv
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.
Thanks for the great work Akif!! Incredible improvement numbers
benchmarks/linear_programming/cuopt/initial_solution_reader.hpp
Outdated
Show resolved
Hide resolved
| line_segment_search.settings = {}; | ||
| bool better_cost_than_parents = | ||
| offspring.get_quality(weights) < | ||
| std::min(other_solution.get_quality(weights), guiding_solution.get_quality(weights)); | ||
| bool better_feasibility_than_parents = offspring.get_feasible() && | ||
| !other_solution.get_feasible() && | ||
| !guiding_solution.get_feasible(); | ||
| bool same_as_parents = | ||
| this->check_if_offspring_is_same_as_parents(offspring, guiding_solution, other_solution); | ||
| // adjust the max_n_of_vars_from_other | ||
| if (n_different_vars > (i_t)ls_recombiner_config_t::max_n_of_vars_from_other) { | ||
| if (same_as_parents) { | ||
| ls_recombiner_config_t::increase_max_n_of_vars_from_other(); | ||
| } else { | ||
| ls_recombiner_config_t::decrease_max_n_of_vars_from_other(); | ||
| } | ||
| } | ||
| if (better_cost_than_parents || better_feasibility_than_parents) { | ||
| CUOPT_LOG_DEBUG("Offspring is feasible or better than both parents"); | ||
| return std::make_pair(offspring, true); | ||
| } |
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.
Nit: there's a fair amount of commonality in the code between the three recombiners - maybe recombine() could be turned into a member of the parent recombiner_t class? To write this common prologue and epilogue once (maybe using CRTP to call ls/fp/bp_recombiner_config_t members from the parent)
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.
This is some larger change. Let's do it in another PR. I created an issue on that:
#208
| point_2.data() + solution.problem_ptr->n_variables, | ||
| point_1.data(), | ||
| delta_vector.begin(), | ||
| [] __device__(const f_t& a, const f_t& b) { return a - b; }); |
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.
Minor nit: I really doubt this has any noticeable effect in practice, but basic scalar types (like int/float/double/char/enums/etc) should be passed by value since passing by const ref/pointer just adds overhead
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.
This must be either Cursor or some copy paste error. Fixed :)
cpp/src/mip/problem/problem.cu
Outdated
|
|
||
| #include <cuda_profiler_api.h> | ||
|
|
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.
debugging leftover?
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.
yes, fixed.
| DEVICE_LOG_DEBUG( | ||
| "For cstr %d, var %d, value %f was not found in the transpose", constraint_id, col, value); |
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.
Did you run into a cusparse transpose issue?
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.
Not related to cusparse, but in integer fixed problem. We are computing the integer fixed problem from the original problem and dynamically fill it, instead of recreating the problem every time. This was to make sure everything matches.
|
/ok to test b7aa9da |
|
/merge |
Description
This PR improves the heuristic quality and performance by following improvements:
Added functionalities:
The improvements resulted in an average gap to optimality after 10mins on H100:
Regular run (B&B and heuristics): from 30% to ~23%
Heuristics only: from 33% to ~25%
On presolved instances: from 26% to ~19%
Issue
partially closes #140
Checklist