-
Notifications
You must be signed in to change notification settings - Fork 113
Fix several bugs appeared in unit testing of JuMP interface #149
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
|
/ok to test 19dba1e |
|
How are you handling problems with zero constraints. |
For MIP, Alice wrote a special code that assigns lower or upper bound based on the sign of the objective coefficient. I guess we can do the same for LP? |
Kh4ster
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!
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.
Please create a new function to get the dual objective in the C API before merging. I also think we should verify the changes around the objective are correct. As this undos past bug fixes.
|
/ok to test 7c4d589 |
|
/ok to test a754b1c |
|
/ok to test d18c8b2 |
| f_t compute_user_objective(const lp_problem_t<i_t, f_t>& lp, f_t obj) | ||
| { | ||
| const f_t user_obj = obj * lp.obj_scale + lp.obj_constant; | ||
| const f_t user_obj = lp.obj_scale * (obj + lp.obj_constant); |
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 think you are changing the convention for how the scaling is done here. I'm not sure why the current convention was chosen. Can you check with @aliceb-nv or @akifcorduk before changing this convention?
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.
The reason we chose the current convention was to handle obj_scale values other than 1 or -1. The new way is equivalent only if obj_scale is 1 or -1.
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.
Removing request changes so you can merge while I am out of office. But I left some comments that would be great if you could address.
Also, it wasn't clear to me if the code handled the case where no constraints are present. If so, could you point me to where this is handled?
akifcorduk
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.
Looks good but one note: the objective offset needs to be correctly handled in case the scaling factor is neither 1 nor -1.
|
/ok to test d77cb4d |
|
/merge |
Description
While testing the JuMP interface, there were several corner cases showed up. Especially for tiny problems and problems without constraints. This PR has several such fixes.
In addition, following things are added:
cuOptGetVersion()in C-APIIssue
closes #148
closes #147
closes #166
Checklist