Add root node presolve using Papilo #234
Conversation
aliceb-nv
left a comment
aliceb-nv
left a comment
There was a problem hiding this comment.
Thanks a lot for the great work Rajesh and Hugo!!
Approving with a few comments :)
Also, could you perhaps add a few tests ensuring third-party presolve works well? Checking infeasibility detection, for example
cpp/src/mip/solve.cu
Outdated
|
|
||
| // FIXME:: reduced_solution.get_stats() is not correct, we need to compute the stats for the | ||
| // full problem | ||
| full_sol.post_process_completed = true; // hack |
There was a problem hiding this comment.
full_problem.post_process_solution() should be able to be called right? Since it's on the original problem with no presolving, it should be a no-op
There was a problem hiding this comment.
I think yes, you're right
There was a problem hiding this comment.
Calling full_problem.post_process_solution requires an unecessary call to preprocess_problem
|
Looks like we at least need a definition added to the lp-milp-settings.rst document, also looking at Python stuff for any gaps .... |
Handle dual post solve.
It has been disabled and it can impact the runtime performance on large models due to lack of parallelism. I'd like to enable it in a follow up PR |
|
/ok to test de00644 |
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Few minor requests
| problem = detail::problem_t<i_t, f_t>(reduced_problem); | ||
| presolve_time = presolve_timer.elapsed_time(); | ||
| CUOPT_LOG_INFO("Third party presolve time: %f", presolve_time); | ||
| } |
There was a problem hiding this comment.
Not for this PR, but we might want to add an setting to write out the presolved model.
|
/ok to test faf0200 |
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Minor change, rest looks good. awesome work @hlinsen
|
/ok to test b490069 |
|
/ok to test acae869 |
|
@hlinsen Lets also add details on papilo being by default available in one and not in another, lets add it in FAQ and any other places where we can highlight this behavior |
|
/ok to test 69a5d2b |
|
/merge |
8 participants
This PR implements:
Notes:
Fixes #277
Credits to: @rg20