-
Notifications
You must be signed in to change notification settings - Fork 112
Fix non-trivial presolve not reporting Optimality #26
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
Fix non-trivial presolve not reporting Optimality #26
Conversation
|
/ok to test |
|
/ok to test |
|
We need to add the bound here too: |
…ed a separate test for trivial presolve
|
/ok to test |
|
/ok to test |
chris-maes
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.
This looks good to me. But I'm not an expert on this code. You might want @akifcorduk to get eyes on this as well.
Thanks for taking the initiative to fix this bug Alice. Your work is much appreciated!
| CUOPT_LOG_INFO("Problem fully reduced at trivial presolve"); | ||
| solution_t<i_t, f_t> sol(*context.problem_ptr); | ||
| sol.set_problem_fully_reduced(); | ||
| context.problem_ptr->post_process_solution(sol); |
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.
Dumb question: is this where the solution is populated? Where does presolve store the information about the solution when the problem is fully reduced in presolve?
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.
In the case of fully presolved problems the solution is populated by post_process_solution() (which tracks the transformations made by presolve) and additional solving stats information is added when get_solution() is called to return the final mip_solution_t : https://github.com/NVIDIA/cuopt/blob/branch-25.05/cpp/src/mip/solution/solution.cu#L560-L606
|
/ok to test |
| // gets the is_feasible | ||
| bool get_feasible(); | ||
| // gets the is_problem_infeasible | ||
| bool get_problem_infeasible(); |
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.
Why do we remove the infeasible options? The problem can be infeasible right?
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.
I decided to cover both the case of infeasibility and optimality after presolve with set_problem_fully_reduced. get_solution() then determines, based on the status of the solution after post_process, which status to return (Optimal or Infeasible)
This ended up making the code more concise so that's why I went with this :)
| CUOPT_LOG_INFO("Problem proven infeasible in presolve"); | ||
| solution_t<i_t, f_t> sol(*context.problem_ptr); | ||
| sol.set_problem_infeasible(); | ||
| sol.set_problem_fully_reduced(); |
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.
If we decide to keep infeasible, here it makes more sense to keep set_problem_infeasible
|
/merge |
The non-trivial presolve did not report optimality if the problem was fully reduced. Fixed by this PR
Closes https://github.com/rapidsai/cuopt/issues/2507