-
Notifications
You must be signed in to change notification settings - Fork 87
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
Interface from IIS. #1056
Interface from IIS. #1056
Conversation
Functionality only available in Xpress for now.
Codecov Report
@@ Coverage Diff @@
## master #1056 +/- ##
==========================================
- Coverage 95.53% 95.48% -0.06%
==========================================
Files 100 100
Lines 11447 11441 -6
==========================================
- Hits 10936 10924 -12
- Misses 511 517 +6
Continue to review full report at Codecov.
|
Thanks for the pointer. Is it required to add tests for this? I'm not sure exactly what to test… And it looks like not all attributes are tested either. |
Oops, I did comments on my phone but forgot to click on "Submit", you should see them now, thanks for the reminder. |
GitHub should really warn you more when you do not post all your messages, but at least they were kept! |
I just did this in the last commit. I also added an enum for the status, so that all the information the solver gives is accessible through MOI. |
@blegat I just changed the value for the enum. However, for the indenting, you had a look at an old version :). |
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 vote we: (i) keep this API as simple as possible; and (ii) rename the enum's to have CONFLICT
in their name to avoid ambiguity with other enums (e.g., FEASIBLE
could be a termination status, and INCLUDED
and EXCLUDED
are pretty broad terms).
I modified the PR accordingly. Indeed, having just one conflict simplifies things quite a bit. |
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.
Yes, simplicity is good.
Still needs some tests and additions to the documentation.
What tests would be needed, here? I'm not sure what to test for MOI… Or you mean a few things for the solver to check their compatibility in MOIT? For the documentation, is there anything to add in |
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.
Add the new types in the docs
Is this enough? |
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.
Thanks, looks good to me once CI passes.
By the way, should I include an entry for NEWS in this PR? |
We usually collect NEWS entry separately as otherwise all PRs will conflict with each other. |
Exact copy from the common bits in the existing solvers, except that
compute_conflict!
now has a !.https://github.com/JuliaOpt/CPLEX.jl/blob/2756458f5f6c02b784e5c2fb4594c930d6febf60/src/MOI/MOI_wrapper.jl#L2745
https://github.com/JuliaOpt/Xpress.jl/blob/master/src/MOI/MOI_wrapper.jl#L3291
https://github.com/JuliaOpt/Gurobi.jl/blob/master/src/MOI_wrapper.jl#L2498
This follows jump-dev/JuMP.jl#1035 (comment).