Sub-MIP recombiner and B&B global variable changes#259
Sub-MIP recombiner and B&B global variable changes#259rapids-bot[bot] merged 31 commits intoNVIDIA:branch-25.10from
Conversation
| CUOPT_LOG_DEBUG( | ||
| "n_vars_from_guiding %d n_vars_from_other %d", n_vars_from_guiding, n_vars_from_other); | ||
| this->compute_vars_to_fix(offspring, vars_to_fix, n_vars_from_other, n_vars_from_guiding); | ||
| auto [fixed_problem, fixed_assignment, variable_map] = offspring.fix_variables(vars_to_fix); |
There was a problem hiding this comment.
Do you know if it would cause size issues if we are to remove fixed variable or apply presolve on the fixed problem?
There was a problem hiding this comment.
What do you mean by "removing fixed variable"?
I don't think it would cause any problems if we applied presolve as long as the conversions are done correctly.
There was a problem hiding this comment.
If we can change the size of the CSR matrix
There was a problem hiding this comment.
We are changing the size of the CSR matrix already. That's the fixed_problem.
| global_variables::mutex_upper.unlock(); | ||
| return upper_bound; | ||
| mutex_upper.lock(); | ||
| const f_t upper_bound_ = upper_bound; |
There was a problem hiding this comment.
Thanks for making these changes @akifcorduk !
One nitpick. We have local variables for lower bound, upper bound, and gap. As well as the member variables that are shared between threads.
I think you used the convention of adding an underscore suffix to local variables (i.e. upper_bound_ or lower_bound_). I'm used to the exact opposite convention---that member variable have an underscore suffix. So I read lower_bound_ as a member variable of the class.
To avoid confusion, would you be ok if we used the underscore prefix for the member variables?
There was a problem hiding this comment.
I agree with Chris :) To me at a first glance, prefix or suffix underscores suggest a member variable
There was a problem hiding this comment.
Actually, I wanted to get rid of this local variables for reading. I wanted to convert all global/shared variables into atomic. I can do it in this PR, or later in another PR. What's your preference @chris-maes ?
There was a problem hiding this comment.
Handled it, let's do the atomic changes in another PR.
| branch_and_bound_settings.integer_tol = context.settings.tolerances.integrality_tolerance; | ||
| // disable B&B logs, so that it is not interfering with the main B&B thread | ||
| branch_and_bound_settings.log.log = false; | ||
| dual_simplex::branch_and_bound_t<i_t, f_t> branch_and_bound(branch_and_bound_problem, |
There was a problem hiding this comment.
I think you should add a callback here. Since any feasible solution in the sub-MIP should be a feasible solution in the original problem. So you want to propagate solutions out of the sub-MIP.
There was a problem hiding this comment.
Good catch. I wanted to implement getting the best primal solution, this way we can get all intermediate solutions.
| fixed_problem.get_host_user_problem(branch_and_bound_problem); | ||
| branch_and_bound_solution.resize(branch_and_bound_problem.num_cols); | ||
| // Fill in the settings for branch and bound | ||
| branch_and_bound_settings.time_limit = sub_mip_recombiner_config_t::sub_mip_time_limit; |
There was a problem hiding this comment.
Probably instead of, or in addition to, a time limit, we should add a node limit to branch and bound
There was a problem hiding this comment.
Why is this a case? Does returning early provide any value? Or does the B&B have diminishing returns in submip context when we run it more than certain number of nodes?
There was a problem hiding this comment.
As we discussed offline, I think using a node limit just makes things more deterministic. We can switch later when the MIP heuristics are deterministic. Fine to use time limit for now.
| namespace diversity_config_t { | ||
| static double time_ratio_on_init_lp = 0.1; | ||
| static double max_time_on_lp = 30; | ||
| static double time_ratio_of_probing_cache = 0.10; | ||
| static double max_time_on_probing = 60; | ||
| static size_t max_iterations_without_improvement = 15; | ||
| static int max_var_diff = 256; | ||
| static size_t max_solutions = 32; | ||
| static double initial_infeasibility_weight = 1000.; | ||
| static double default_time_limit = 10.; | ||
| static int initial_island_size = 3; | ||
| static int maximum_island_size = 8; | ||
| static bool use_avg_diversity = false; | ||
| static double generation_time_limit_ratio = 0.6; | ||
| static double max_island_gen_time = 600; | ||
| static size_t n_sol_for_skip_init_gen = 3; | ||
| static double max_fast_sol_time = 10; | ||
| static double lp_run_time_if_feasible = 15.; | ||
| static double lp_run_time_if_infeasible = 1; | ||
| static bool halve_population = true; | ||
| }; // namespace diversity_config_t |
There was a problem hiding this comment.
Was the loss of the const/constexpr qualifier intended?
Mutable global variables may cause issues
There was a problem hiding this comment.
Yes this was intended. We are changing those constants depending on a environment config variable. They are wrapped in a namespace, so i doubt they will cause issues.
There was a problem hiding this comment.
Okay, makes sense!
Although, ideally to make the semantics clearer I'd see this as a separate struct type, marked as a 'const' member and properly initialized at solver construction
Even if in this context it is unlikely to cause issues, global mutable variables are just inherently alarming especially in a large multithreaded codebase
There was a problem hiding this comment.
If you keep them as global: please mark them as 'static inline' or 'extern'. As it stands, a static variable declared in a header will be instantiated with internal linkage in each separate translation unit; so if one of them is modified in a given .cu file, other .cu files won't see this change
There was a problem hiding this comment.
Okay, i like the idea of member, i will do that.
| global_variables::mutex_upper.unlock(); | ||
| return upper_bound; | ||
| mutex_upper.lock(); | ||
| const f_t upper_bound_ = upper_bound; |
There was a problem hiding this comment.
I agree with Chris :) To me at a first glance, prefix or suffix underscores suggest a member variable
chris-maes
left a comment
There was a problem hiding this comment.
Changes to branch and bound and pseudocosts look good to me.
Awesome work Akif. I'm excited to have sub-MIPping in cuOpt!
|
/merge |
This PR adds a new recombiner: sub-MIP. We use the B&B solver to solve the subproblem with the time limit of the recombiner. This PR also runs both local searches (line segment and FJ) one after another, instead of running either of them with 50% probability. This PR also changes all the global variables in B&B to class member variables. This way, multiple B&B instances could run in parallel in the same process, including the sub-MIP recombiner. Authors: - Akif ÇÖRDÜK (https://github.com/akifcorduk) Approvers: - Chris Maes (https://github.com/chris-maes) URL: NVIDIA#259
This PR adds a new recombiner: sub-MIP. We use the B&B solver to solve the subproblem with the time limit of the recombiner. This PR also runs both local searches (line segment and FJ) one after another, instead of running either of them with 50% probability.
This PR also changes all the global variables in B&B to class member variables. This way, multiple B&B instances could run in parallel in the same process, including the sub-MIP recombiner.