MOI/LQOI basic support for ILP #181

IssamT · 2018-08-05T20:04:37Z

No description provided.

odow

Awesome. Mainly lots of style things. See
jump-dev/GLPK.jl#59
jump-dev/Gurobi.jl#146

odow · 2018-08-06T07:25:56Z

src/MOIWrapper.jl

@@ -0,0 +1,283 @@
+export CplexOptimizer


Refactor CplexOptimizer to CPLEX.Optimizer and remove the export.

odow · 2018-08-06T07:26:07Z

src/MOIWrapper.jl

+const MOI  = LQOI.MOI
+
+const SUPPORTED_OBJECTIVES = [
+    LQOI.Linear


Add LQOI.SinVar

odow · 2018-08-06T07:26:30Z

src/MOIWrapper.jl

+    for (name,value) in kwargs
+        set_param!(env, string(name), value)
+    end
+    m = CplexOptimizer(nothing)


Style: always name the optimizer model

odow · 2018-08-06T07:26:48Z

src/MOIWrapper.jl

+    return m
+end
+
+LQOI.supported_constraints(s::CplexOptimizer) = SUPPORTED_CONSTRAINTS


Style: don't name s it it isn't used

odow · 2018-08-06T07:27:07Z

src/MOIWrapper.jl

+LQOI.backend_type(m::CplexOptimizer, ::MOI.Nonnegatives)         = Cchar('E')
+
+# TODO - improve single type
+function LQOI.change_variable_bounds!(instance::CplexOptimizer, 


Style: refactor instance to model

odow · 2018-08-06T07:28:15Z

src/MOIWrapper.jl

+function LQOI.set_linear_objective!(instance::CplexOptimizer, 
+        columns::Vector{Int}, coefficients::Vector{Float64})
+
+    n = num_var(instance.inner)    


Style: no one-letter variable names

But n and m are often used in linear programming. And these are local vars in functions that always fully fit in a laptop screen; so if one doesn't know what the variable is, he can just look above.

odow · 2018-08-06T07:29:02Z

src/MOIWrapper.jl

+end
+
+function LQOI.get_variable_primal_solution!(instance::CplexOptimizer, result)
+    fill_solution(instance.inner, result)


Nit: get_ instead of fill_?

Since there are already get functions with the same name, the name should be different for inplace functions. following our discussion yesterday, I will use get_ with !

odow · 2018-08-06T07:29:22Z

src/MOIWrapper.jl

+end
+
+function LQOI.get_objective_value(instance::CplexOptimizer)
+    get_objval(instance.inner)


Style: all functions that explicitly return something should use return

odow · 2018-08-06T07:30:29Z

src/cpx_constrs.jl

@@ -156,6 +166,21 @@ function get_constr_senses(model::Model)
    return senses
 end

+function chg_sense!(model::Model, indices::IVec, senses::CVec)


Style: I think if we are wrapping new CPLEX functions, we should use exactly the CPLEX name. So refactor chg_sense! to chgsense.

I only added few functions, so I tried to keep the style consistent with the rest of the wrapper functions. Plus I think if we use underscores everywhere else in julia code, it's better to keep those underscores. I opened an issue to see what the others think of this jump-dev/JuMP.jl#1409

odow · 2018-08-06T07:31:49Z

src/cpx_vars.jl

@@ -78,6 +78,22 @@ function add_var!(model::Model, constridx::Vector, constrcoef::Vector, l::Vector
    return add_var!(model, ivec(constridx), fvec(constrcoef), fvec(l), fvec(u), fvec(objcoef))
 end

+function get_varLB(model::Model, col::Cint)


Style: getlb

I think we should keep the name get_lb for the function that does exactly the same as cpxgetlb, namely getting lowerbounds for a range of columns and not for just one column.

IssamT · 2018-08-14T09:18:38Z

@odow, are you fine with the current names?

odow

Yes I like c_api_xxx. However, maybe we should put all of the c_api_xxx functions in a new c_api.jl file for now. In the long-term, I would like to remove/deprecate many of the existing CPLEX functions.

odow · 2018-08-14T21:48:07Z

src/cpx_constrs.jl

    if stat != 0
        throw(CplexError(model.env, stat))
    end
 end

+function c_api_getrhs(model::Model, row_start::Cint, row_end::Cint)


I think users should have to pass in the rhs vector as well.

function c_api_getrhs(model::Model, rhs::Vector{Cdouble}, row_start::Cint, row_end::Cint)

@odow, I agree with you. higher layers can benefit from that doing in place modification even if it's not the case of LQOI for this specific function. I will update it.

IssamT · 2018-08-15T10:01:24Z

How about renaming all files like

cpx_model.jl -> c_api_model.jl
cpx_vars.jl -> c_api_vars.jl

and move the helper functions that we want to keep as few as possible from those file to a single file

helpers_c_api.jl

IssamT · 2018-08-24T10:42:21Z

@odow should we go with the above renaming?

odow · 2018-08-24T20:13:50Z

I think for now, leave the current cplex unmodified and place any new functions in a new file. We can make another pr that does a tidy

IssamT · 2018-08-24T21:15:02Z

I also renamed some of the old ones used also by Mathprogbase when they matched the C API. If you prefer doing the renaming in another PR, then I think it's better to keep the functions in the same files. otherwise we will have subsets of functions organized differently without any reason.

odow

Okay, lets leave the functions where they are for now. A few comments.

It needs to be upgraded to LQOI v0.3.
There are lot of style/naming things (e.g., in c_api_getrows, what is mbegin, m, surplus_p etc.)
v0.7 support?
missing a few MOI tests.

odow · 2018-08-24T22:45:56Z

src/cpx_constrs.jl

    ncons = @cpx_ccall(getnumrows, Cint, (
                       Ptr{Void},
                       Ptr{Void}
                       ),
                       model.env.ptr, model.lp)
    return ncons
 end
+num_constr = c_api_getnumrows # Needed by MathProgBase Interface


I initially missed this, it might be better to go num_constr(model::Model) = c_api_getnumrows(model)

odow · 2018-08-24T22:46:20Z

src/cpx_constrs.jl

    if stat != 0
        throw(CplexError(model.env, stat))
    end
 end

+function c_api_getrhs(model::Model, rhs::Vector{Cdouble}, row_start::Cint, row_end::Cint)
+    ncons = 1


odow · 2018-08-24T22:47:38Z

src/cpx_solve.jl

@@ -79,28 +80,53 @@ function get_objval(model::Model)
  end
  return objval[1]
 end
+get_objval = c_api_getobjval


get_objval(model::Model) = c_api_getobjval(model)

odow · 2018-08-24T22:48:29Z

src/cpx_solve.jl

-  nvars = num_var(model)
-  x = Vector{Cdouble}(nvars)
+function c_api_solninfo(model::Model)    
+  solnmethod_p = [Cint(-1)]


You can make these solnmethod_p = Ref{Cint}(), and then return solnmethod_p[]. This avoids allocating a vector.

odow · 2018-08-24T22:49:50Z

src/cpx_solve.jl

+    return @cpx_ccall(getstat, Cint, (Ptr{Void}, Ptr{Void}), 
+                      model.env.ptr, model.lp)
+end
+get_status_code = c_api_getstat


get_status_code(model::Model) = c_api_getstat(model)

odow · 2018-08-24T22:50:44Z

src/cpx_vars.jl

    nvar = @cpx_ccall(getnumcols, Cint, (
                      Ptr{Void},
                      Ptr{Void}
                      ),
                      model.env.ptr, model.lp)
    return(nvar)
 end
+num_var = c_api_getnumcols


Here again.

odow · 2018-08-24T22:51:20Z

test/MOIWrapper.jl

@@ -0,0 +1,36 @@
+using CPLEX, Base.Test, MathOptInterface, MathOptInterface.Test


You should test this on v0.7 as well. It will need Compat.Test instead of Base.Test, or we can leave that for another PR.

You also don't need the using MathOptInterface.Test

odow · 2018-08-24T22:51:55Z

test/MOIWrapper.jl

+    solver = CPLEX.Optimizer()
+    MOIT.intlineartest(solver, intconfig, ["int2", "int3"]) 
+    # 3 is ranged, 2 has sos
+end


Go see the Gurobi and GLPK tests, there are a few missing here.

odow · 2018-08-24T22:52:56Z

src/cpx_model.jl

-    else
-        error("CPLEX: problem object or environment does not exist")
+        return :Min
+    else 


Why did we drop the check for sense_int == -1 and the else error?

odow · 2018-08-24T22:53:39Z

src/MOIWrapper.jl

+
+function MOI.free!(m::Optimizer)
+    # close_CPLEX(model.inner)   
+end


This method is not needed.

IssamT · 2018-08-25T16:04:17Z

I think we can keep the upgrade to julia 0.7 for another PR, since the MathProgBase interface is also failing it.

odow

Do the tests pass?

odow · 2018-08-26T00:58:38Z

src/MOIWrapper.jl

+    n = num_var(model.inner)    
+    all_coefs = zeros(Float64, n)    
+    for (col, coef) in zip(columns, coefficients)
+        all_coefs[col] = coef


Do the tests pass? This doesn't account for duplicate coefficients. Should be all_coefs[col] += coef

odow · 2018-08-26T01:00:02Z

test/MOIWrapper.jl

+            MOIT.TestConfig()
+        )
+    end
+end


You should add the modellike tests
https://github.com/JuliaOpt/Gurobi.jl/blob/9bf2a76e27c82198ab4b2ce387944d666e5618eb/test/MOIWrapper.jl#L80-L97

odow · 2018-08-26T01:01:41Z

test/MOIWrapper.jl

+        "solve_affine_interval",     # not implemented
+        "solve_qp_edge_cases",       # not implemented
+        "solve_qcp_edge_cases",      # not implemented
+        "solve_objbound_edge_cases", # TODO fix the non zero constant case


This should be addressed with:
https://github.com/JuliaOpt/LinQuadOptInterface.jl/blob/e3737c0cacb8d60a8fd2609ceb3e024dfcd90773/src/solver_interface.jl#L326
https://github.com/JuliaOpt/LinQuadOptInterface.jl/blob/e3737c0cacb8d60a8fd2609ceb3e024dfcd90773/src/solver_interface.jl#L375

odow · 2018-08-26T01:02:10Z

test/MOIWrapper.jl

@@ -0,0 +1,36 @@
+using CPLEX, Base.Test, MathOptInterface, MathOptInterface.Test


You also don't need the using MathOptInterface.Test

odow · 2018-08-29T22:54:27Z

test/MOIWrapper.jl

+    solver = CPLEX.Optimizer()
+    # TODO add a test config to disable testing problem name.
+    # @testset "nametest" begin
+    #     MOIT.nametest(solver)


Why does the nametest fail? Since CLPEX uses LQOI it should pass...

You're right. My bad.

IssamT · 2018-09-02T06:58:15Z

I think it s better to do the update for MOI 0.6 in a separate PR, to keep a version for comparaison. Any minor issues left can be discussed in the MOI 0.6 update PR.

joaquimg · 2018-09-03T22:21:12Z

I agree

odow · 2018-09-03T22:40:29Z

Okay, I have tagged a release of CPLEX prior to merging this: JuliaLang/METADATA.jl#17661

I'll squash and merge this, then you can make another PR updating to MOI v0.6 and Julia v0.7. Then we can tag as v0.4.0

MOI/LQOI support for ILP

2528094

odow reviewed Aug 6, 2018

View reviewed changes

IssamT added 2 commits August 6, 2018 14:13

style fixing

557c56b

updates following jump-dev/JuMP.jl#1409

1135c7f

odow reviewed Aug 14, 2018

View reviewed changes

making c_api_getrhs in place

0289865

odow reviewed Aug 24, 2018

View reviewed changes

fixes thanks to @odow comments

bc4f867

odow reviewed Aug 26, 2018

View reviewed changes

IssamT added 2 commits August 29, 2018 22:25

a few fixes

1797904

removed a useless using

ffe67b9

odow reviewed Aug 29, 2018

View reviewed changes

enabled name test

3a3bf90

odow merged commit 9ac7fbf into jump-dev:master Sep 3, 2018

This was referenced Sep 3, 2018

LQOI #146

Closed

WIP: MOI #139

Closed

odow mentioned this pull request Oct 11, 2018

Help Wanted: Update for MOI #180

Closed

		@@ -0,0 +1,36 @@
		using CPLEX, Base.Test, MathOptInterface, MathOptInterface.Test

MOI/LQOI basic support for ILP #181

MOI/LQOI basic support for ILP #181

Conversation

IssamT commented Aug 5, 2018

odow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IssamT Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IssamT commented Aug 14, 2018

odow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IssamT commented Aug 15, 2018

IssamT commented Aug 24, 2018

odow commented Aug 24, 2018

IssamT commented Aug 24, 2018

odow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IssamT commented Aug 25, 2018

odow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IssamT commented Sep 2, 2018

joaquimg commented Sep 3, 2018

odow commented Sep 3, 2018

IssamT Aug 6, 2018 •

edited

Loading