-
Notifications
You must be signed in to change notification settings - Fork 122
Fix inversion of constraint bounds in conditional bounds presolve #75
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
| auto new_c_lb = max(c_lb, min_a); | ||
| auto new_c_ub = min(c_ub, max_a); | ||
| i_t infeas = check_infeasibility<i_t, f_t>( | ||
| min_a, max_a, new_c_lb, new_c_ub, pb.tolerances.absolute_tolerance, 0); |
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.
pb.tolerances.absolute_tolerance is pretty large: 1e-4 if I remember correctly. I think ideally this would be around 1e-6. Maybe we should introduce a new tolerance here that we can remove later once we bring the absolute tolerance down to 1e-6.
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.
relative_tolerance default value is set to 1e-6. Do you want to just use that? If not, what should I name the new tolerance as?
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.
Maybe we should add a tolerance called presolve_absolute_tolerance = 1e-6. We can remove it later when we set the absolute_tolerance = 1e-6.
|
Thanks for the fix @kaatish . Would you also be able to add a tolerance to We also should consider if we should declare the problem infeasible (rather than throwing an exception) if |
Same as above? Tolerance of 1e-6?
I can return a bool from check_bounds_sanity and return false from run_presolve if the check fails. What you are proposing is to add those checks (apart from the checks we already have) before we run trivial_presolve? |
Yes, I would use 1e-6. You can refer to it as
No need to add any additional checks. I'm just proposing we mark the problem as infeasible (rather than throwing an exception) if the check fails before presolve. During presolve if the bounds cross, we should already return Infeasible before calling this check. |
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.
LGTM
|
/merge |
Updating the constraint bounds based on minimal and maximal activity can sometimes cause the update constraint lower bounds to be greater than constraint upper bounds even if the constraint is feasible within tolerance.
This PR fixes the constraint bounds to the same value if this happens.