Moiwrapper #54

vitornesello · 2018-06-18T17:57:45Z

No description provided.

mlubin · 2018-06-18T18:22:06Z

Very cool!

CC @IssamT

IssamT · 2018-06-18T18:54:53Z

@mlubin I've already seen the code. We share the same office ;)

mlubin · 2018-06-18T18:59:46Z

I expected some correlation given Vitor's location :)

I'm currently traveling but I'll try to take a look at this in the next couple days.

vitornesello · 2018-06-18T19:10:14Z

We only have two issues now.

The first one is whether it should be added to the MOI test config the option of saying that the optimizer cannot get MOI.ConstraintPrimal. For instance, this feature is not implemented in CbcCInterface.jl.

The second one is that it seems to be hard to get MOI.ResultCount with Cbc. Either it returns one solution or it does not return, but neither the status codes nor the solution vector really help. I am trying to use the objective function value but I am not sure if it is safe.

odow

Good stuff! I made quite a few comments. Most are just style or functions in Julia you might not be aware of.

The first one is whether it should be added to the MOI test config the option of saying that the optimizer cannot get MOI.ConstraintPrimal

Just exclude any tests that need it for now.

The second one is that it seems to be hard to get MOI.ResultCount with Cbc

What happens if you query the solution vector when Cbc has been interrupted or the problem is infeasible?

odow · 2018-06-19T00:13:46Z

src/MOIWrapper.jl

@@ -0,0 +1,397 @@
+export CbcOptimizer, copy!, optimize!, get


You should only export CbcOptimizer

odow · 2018-06-19T00:17:41Z

src/MOIWrapper.jl

+            col_lb[colIdx] = -Inf
+            col_ub[colIdx] = Inf
+            obj[colIdx] = 0.0
+        end


Instead of this just use

row_lb = fill(-Inf, nbRows) obj = zeros(Float64, nbCols) constraint_matrix = sparse(Int[], Int[], Float64[], nbRows, nbCols) # see note about sparse matrix

(Ditto with the other fields.)

odow · 2018-06-19T00:21:51Z

src/MOIWrapper.jl

+mutable struct CbcModelFormat
+    nbRows::Int
+    nbCols::Int
+    constraint_matrix::Array{Float64,2}


You probably want to make this a sparse array instead of a dense array. See ? SparseMatrixCSC

odow · 2018-06-19T00:23:55Z

src/MOIWrapper.jl

+        mapping.varmap[varIndex[i]] = MOI.VariableIndex(i)
+    end
+
+    zeroOneIndices = Vector{Int}(0)


You can also go zeroOneIndices = Int[]

odow · 2018-06-19T00:24:10Z

src/MOIWrapper.jl

+    zeroOneIndices = Vector{Int}(0)
+    integerIndices = Vector{Int}(0)
+    listOfConstraints = MOI.get(userOptimizer, MOI.ListOfConstraints())
+    nbRows = 0;


Don't need the ;

odow · 2018-06-19T00:29:28Z

src/MOIWrapper.jl

+
+function MOI.cantransformconstraint(model::CbcOptimizer, c::MOI.ConstraintIndex{F}, ::Type{S}) where {F<:MOI.AbstractFunction, S<:MOI.AbstractSet}
+    return false
+end


I think you can leave out these functions as the default should be false

odow · 2018-06-19T00:30:09Z

src/MOIWrapper.jl

+
+function MOI.set!(cbcOptimizer::CbcOptimizer, object::MOI.ObjectiveSense, sense::MOI.OptimizationSense)
+    sense == MOI.MinSense && CbcCI.setObjSense(cbcOptimizer.inner, 1)
+    sense == MOI.MaxSense && CbcCI.setObjSense(cbcOptimizer.inner, -1)


There is also a MOI.FeasibilitySense

odow · 2018-06-19T00:30:57Z

src/MOIWrapper.jl

+const CANGETCONSATTR = []
+
+function MOI.canget(cbcOptimizer::CbcOptimizer, object::MOI.PrimalStatus)
+    object.N >= 2 && return false


object.N != 1 && return false?

odow · 2018-06-19T00:31:49Z

src/MOIWrapper.jl

+
+function MOI.get(cbcOptimizer::CbcOptimizer, object::MOI.VariablePrimal, ref::Vector{MOI.VariableIndex})
+    variablePrimals = CbcCI.getColSolution(cbcOptimizer.inner)
+    askedPrimals = Vector{Float64}()


Just use generator syntax instead of creating an empty array then looping:
return [variablePrimals[vi.value] for vi in ref] or
return map(vi->variablePrimals[vi.value], ref)

odow · 2018-06-19T00:37:14Z

src/MOIWrapper.jl

+
+
+function MOI.get(cbcOptimizer::CbcOptimizer, object::MOI.ResultCount)
+    isProvenInfeasible(cbcOptimizer.inner) && return 0


What about unbounded models? Does Cbc return a solution?

blegat · 2018-06-19T11:30:31Z

src/MOIWrapper.jl

+    listOfConstraints = MOI.get(userOptimizer, MOI.ListOfConstraints())
+    nbRows = 0;
+    for (F,S) in listOfConstraints
+        (F,S) in SUPPORTED_CONSTRAINTS || return MOI.CopyResult(MOI.CopyUnsupportedConstraint, "Cbc MOI Interface does not support constraints of type " * (F,S) * ".", nothing)


You might want do do the loop in a separate function so to make it type stable in this separate function like in
https://github.com/JuliaOpt/MathOptInterface.jl/blob/master/src/Utilities/copy.jl#L123-L125

blegat · 2018-06-19T11:31:19Z

src/MOIWrapper.jl

+    listOfConstraints = MOI.get(userOptimizer, MOI.ListOfConstraints())
+    nbRows = 0;
+    for (F,S) in listOfConstraints
+        (F,S) in SUPPORTED_CONSTRAINTS || return MOI.CopyResult(MOI.CopyUnsupportedConstraint, "Cbc MOI Interface does not support constraints of type " * (F,S) * ".", nothing)


You can replace (F, S) in SUPPORTED_CONSTRAINTS by MOI.supportsconstraint(cbcOptimizer, F, S)

blegat · 2018-06-19T11:34:19Z

src/MOIWrapper.jl

+
+## supports constraints
+
+function MOI.supportsconstraint(cbcOptimizer::CbcOptimizer, F::Type{<:MOI.AbstractFunction}, S::Type{<:MOI.AbstractScalarSet})


It would be more efficient to do check if it is supported at compile time by doing

MOI.supportsconstraint(::CbcOptimizer, ::Type{<:Union{...}}, ::Type{<:Union{...}}) = true

and it would also make the result of supportsconstraint inferrable so calls to supportsconstraint can be optimized out by the compiler.

blegat · 2018-06-19T11:35:13Z

src/MOIWrapper.jl

+end
+
+function MOI.supports(cbcOptimizer::CbcOptimizer, object::MOI.ObjectiveFunction{T}) where T <: MOI.AbstractFunction
+    return T in SUPPORTED_OBJECTIVES


Same here, it's probably better to do

MOI.supports(...) = true

and specify what is supported in the type

blegat · 2018-06-19T11:36:31Z

src/MOIWrapper.jl

+# empty!
+
+function MOI.empty!(cbcOptimizer::CbcOptimizer)
+    cbcOptimizer = CbcOptimizer()


This does nothing, you need to modify the value of cbcOptimizer given as argument. Doing cbcOptimizer = CbcOptimizer() changes the object to which the local variable cbcOptimizer refers to but does not modify it

blegat · 2018-06-19T11:37:10Z

src/MOIWrapper.jl

+end
+
+
+function MOI.canget(cbcOptimizer::CbcOptimizer, object::Union{MOI.AbstractModelAttribute, MOI.AbstractOptimizerAttribute})


Again, this can be handled by an Union

odow

I'm not sure what we should do about the binary variable bounds.

Does it make sense to add a binary constraint to a variable if it has a variable bound? @blegat @joaquimg ???

odow · 2018-06-20T02:58:44Z

src/MOIWrapper.jl

+mutable struct CbcModelFormat
+    nbRows::Int
+    nbCols::Int
+    constraint_matrix::SparseMatrixCSC{Float64,Int32}


This should be SparseMatrixCSC{Float64, Int}

odow · 2018-06-20T03:02:16Z

src/MOIWrapper.jl

+
+mutable struct CbcOptimizer <: MOI.AbstractOptimizer
+    inner::CbcModel
+    # env


Is this needed?

The mutability is necessary because of the empty! function, which modifies the pointer CbcOptimizer.inner.

@odow might be refering to the # env comment

odow · 2018-06-20T03:03:15Z

src/MOIWrapper.jl

+        row_ub::VecOrNothing,
+    These are needed by function loadProblem of CbcCInterface.
+
+```


Did you mean to make this a docstring with """ instead of ```?

odow · 2018-06-20T03:03:48Z

src/MOIWrapper.jl

+
+
+
+#


Trailing comment

blegat · 2018-06-20T09:42:36Z

src/MOIWrapper.jl

+end
+
+"""
+    Create inner model cbcOptimizer based on abstract model userOptimizer provided by user.


If you indent, in markdown, it means that this is code.
You should rather put an indented signature followed by an unindented description :)

mlubin · 2018-06-23T19:01:12Z

src/MOIWrapper.jl

+end
+
+struct CbcModelFormat
+    nbRows::Int


I know that CbcCInterface has weird code, but camelCase shouldn't be used in new Julia code. num_rows instead.

mlubin · 2018-06-23T19:01:23Z

src/MOIWrapper.jl

+
+struct CbcModelFormat
+    nbRows::Int
+    nbCols::Int


mlubin · 2018-06-23T19:02:00Z

src/MOIWrapper.jl

+end
+
+
+function loadConstraint(ci::MOI.ConstraintIndex, cbcModelFormat::CbcModelFormat, mapping::MOIU.IndexMap,


cbc_model_format instead of cbcModelFormat.

mlubin · 2018-06-23T19:03:14Z

src/MOIWrapper.jl

+end
+
+
+function loadConstraint(ci::MOI.ConstraintIndex, cbcModelFormat::CbcModelFormat, mapping::MOIU.IndexMap,


load_constraint instead of loadConstraint

mlubin · 2018-06-23T19:05:27Z

src/MOIWrapper.jl

+    mapping::MOIU.IndexMap, f::MOI.ScalarAffineFunction, s::MOI.GreaterThan)
+    for term in f.terms
+        cbcModelFormat.constraint_matrix[mapping.conmap[ci].value,
+        mapping.varmap[term.variable_index].value] += term.coefficient


This is a super inefficient way to build a sparse matrix. Instead, keep a collection of the (row, column, value) triplets and call sparse() to build the matrix at the end.

mlubin · 2018-06-23T19:06:44Z

src/MOIWrapper.jl

+function loadObj(cbcModelFormat::CbcModelFormat, mapping::MOIU.IndexMap,
+    f::MOI.ScalarAffineFunction)
+    for term in f.terms
+        cbcModelFormat.obj[mapping.varmap[term.variable_index].value] += term.coefficient


This assumes that obj is all zeros at the start, right? Add a comment mentioning this assumption.

When cbc_model_format is created everything is initialized to 0.0. So I do += to handle cases where the user sets an objective function like $x_1 + x_2 + x_1$. These cases are tested in contlinear.jl.

That's all good, but please add a comment about this assumption right here.

mlubin · 2018-06-23T19:07:25Z

src/MOIWrapper.jl

+    end
+end
+
+    """


Why is the docstring indented? It should be aligned with the function below.

mlubin · 2018-06-23T19:08:32Z

src/MOIWrapper.jl

+function MOI.set!(cbcOptimizer::CbcOptimizer, object::MOI.ObjectiveSense, sense::MOI.OptimizationSense)
+    if sense == MOI.MaxSense
+        CbcCI.setObjSense(cbcOptimizer.inner, -1)
+    end ## Other senses are set as minimization (cbc default)


What happens this function is called with MaxSense and then MinSense? The MinSense will never be set.

mlubin · 2018-06-23T19:10:49Z

src/MOIWrapper.jl

+    MOI.set!(cbcOptimizer, MOI.ObjectiveSense(), sense)
+
+    ## Load the problem to Cbc
+    CbcCI.loadProblem(cbcOptimizer.inner, cbcModelFormat.constraint_matrix,


Do we need to keep the constraint_matrix in memory after it's loaded into Cbc?

This object is local to the function copy! where it is defined, isn't it?

You're right, I missed that.

mlubin · 2018-06-23T19:11:45Z

test/MOIWrapper.jl

+    @testset "orderedindicestest" begin
+        MOIT.orderedindicestest(optimizer)
+    end
+    # @testset "canaddconstrainttest" begin ## do not pass due to vector of fariables


Typo: fariables -> variables (also below)

mlubin

This is a big improvement from before. A few small issues left.

mlubin · 2018-06-30T12:29:51Z

src/MOIWrapper.jl

@@ -0,0 +1,358 @@
+export CbcOptimizer


CbcCInterface and CbcSolverInterface both define define submodules. We should be consistent, either turn this code into a submodule or drop the submodules from the other files.

The consistency is preferred also among the different solvers. maybe we should decide about having the wrapper as a submodule or not, and add a sentence here

http://www.juliaopt.org/MathOptInterface.jl/stable/apimanual.html#Package-Naming-1

mlubin · 2018-06-30T12:44:29Z

src/MOIWrapper.jl

+
+
+using MathOptInterface
+using Cbc.CbcCInterface


I think it's better to use a relative import, i.e., using .CbcCInterface.

mlubin · 2018-06-30T12:44:50Z

src/Cbc.jl

 include("CbcSolverInterface.jl")
+include("MOIWrapper.jl")

 using Cbc.CbcMathProgSolverInterface


Also try changing this to a relative import: using .CbcMathProgSolverInterface.

mlubin · 2018-06-30T12:47:30Z

src/MOIWrapper.jl

+end
+
+"""
+Receive a cbc_optimizer which contains the pointer to the cbc C object and instanciate the object cbc_model_format::CbcModelFormat based on user_optimizer::AbstractModel (also provided by the user).


Typo: instantiate.

mlubin · 2018-06-30T12:49:09Z

src/MOIWrapper.jl

+    end
+end
+
+"""


The first line of the doc string should repeat the signature of the method, e.g.,

""" foo(a, b) This is a docstring. """

This is because when you ask for help on MOI.copy!, it concatenates all the docstrings for the possible methods (try it).

mlubin · 2018-06-30T12:58:20Z

src/MOIWrapper.jl

+
+        ci = MOI.get(user_optimizer, MOI.ListOfConstraintIndices{F,S}())
+
+        if S == MOI.ZeroOne


This can be restructured for clarity. Right now it's a bit indirect that these cases can only happen when F is SingleVariable. E.g.,

if F == MOI.SingleVariable if S == MOI.ZeroOne ... elseif S == MOI.Integer ... end ## Single variables are treated by bounds in Cbc, so no ## need to add a row else ... # content of F != MOI.SingleVariable case end

mlubin · 2018-06-30T13:03:20Z

src/MOIWrapper.jl

+## Set functions
+
+function MOI.write(cbc_optimizer::CbcOptimizer, filename::String)
+    writeMps(cbc_optimizer.inner, filename)


Error if !endswith("filename", "mps").

mlubin · 2018-06-30T13:06:19Z

test/MOIWrapper.jl

+        "solve_blank_obj",
+        "solve_qcp_edge_cases",
+        "solve_qp_edge_cases",
+        "solve_affine_interval", ## cannot get with strings


Why is this a problem? Doesn't CachingOptimizer deal with getting by string?

mlubin · 2018-06-30T13:07:23Z

src/MOIWrapper.jl

+    MOI.set!(cbc_optimizer, MOI.ObjectiveSense(), sense)
+
+    ## Load the problem to Cbc
+    CbcCI.loadProblem(cbc_optimizer.inner, sparse(cbc_model_format.row_idx, cbc_model_format.col_idx,


Clear row_idx, col_idx, and values after this call to save memory?

mlubin · 2018-07-28T01:43:38Z

@vitornesello @IssamT is this ready to merge?

vitornesello · 2018-07-28T08:14:13Z

@mlubin Yes. We have been using it already and we had no problems so far.

mlubin · 2018-07-28T12:06:35Z

Thanks!!

vitornesello added 5 commits June 14, 2018 19:13

first version of the wrapper, supporting only one type constraint

5868a6d

adding simple tests for the first version

a09d1d0

adding more constraint types and tests

caddd94

adding more constraint types and tests

5de842c

first MOI wrapper for cbc many tests passing

4cd48be

odow reviewed Jun 19, 2018

View reviewed changes

blegat reviewed Jun 19, 2018

View reviewed changes

vitornesello added 3 commits June 19, 2018 16:37

Adding integer and unit tests and fixing suggestions

0f53362

cleaning the code

b6473a7

more cleanning of the code

da10146

odow reviewed Jun 20, 2018

View reviewed changes

vitornesello added 2 commits June 20, 2018 08:24

small fixes sugested by Oscar Dowson

cc0f58e

making CbcModelFormat immutable

93ace81

blegat reviewed Jun 20, 2018

View reviewed changes

vitornesello added 2 commits June 20, 2018 13:12

removing useless comment and changing indentation of docstring

2cfb7b4

adding write function

c4dd557

mlubin reviewed Jun 23, 2018

View reviewed changes

src/MOIWrapper.jl Outdated

struct CbcModelFormat

nbRows::Int

nbCols::Int

Copy link

Member

mlubin Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

num_cols

mlubin reviewed Jun 23, 2018

View reviewed changes

src/MOIWrapper.jl Outdated

end

end

"""

Copy link

Member

mlubin Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the docstring indented? It should be aligned with the function below.

mlubin reviewed Jun 23, 2018

View reviewed changes

vitornesello added 2 commits June 25, 2018 11:58

fixes suggested by mlubin

f16ac3a

more fixes suggested by mlubin

7dc708b

mlubin reviewed Jun 30, 2018

View reviewed changes

a few fixes

bb35b90

odow mentioned this pull request Jul 3, 2018

getbasis not implemented #55

Closed

IssamT added 2 commits July 11, 2018 14:54

added MOI to the Require file

da555ce

excluding linear11 test

8107159

mlubin merged commit d0fe5ab into jump-dev:master Jul 28, 2018

simonschoelly mentioned this pull request Aug 29, 2018

Added fixes for Julia 1.0 JuliaGraphs/LightGraphsFlows.jl#19

Merged



		function MOI.get(cbcOptimizer::CbcOptimizer, object::MOI.ResultCount)
		isProvenInfeasible(cbcOptimizer.inner) && return 0


		## supports constraints

		function MOI.supportsconstraint(cbcOptimizer::CbcOptimizer, F::Type{<:MOI.AbstractFunction}, S::Type{<:MOI.AbstractScalarSet})

		end


		function MOI.canget(cbcOptimizer::CbcOptimizer, object::Union{MOI.AbstractModelAttribute, MOI.AbstractOptimizerAttribute})

		end


		function loadConstraint(ci::MOI.ConstraintIndex, cbcModelFormat::CbcModelFormat, mapping::MOIU.IndexMap,

+                  end
+              end
+                  """


		ci = MOI.get(user_optimizer, MOI.ListOfConstraintIndices{F,S}())

		if S == MOI.ZeroOne

Moiwrapper #54

Moiwrapper #54

Uh oh!

Conversation

vitornesello commented Jun 18, 2018

Uh oh!

mlubin commented Jun 18, 2018

Uh oh!

IssamT commented Jun 18, 2018

Uh oh!

mlubin commented Jun 18, 2018

Uh oh!

vitornesello commented Jun 18, 2018

Uh oh!

odow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blegat Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlubin Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blegat Jun 19, 2018 •

edited

Loading

mlubin Jun 23, 2018 •

edited

Loading