WIP : MOI wrapper (based on LQOI) #27

IssamT · 2018-05-29T10:56:24Z

No description provided.

odow · 2018-06-14T18:24:19Z

test/MOIWrapper.jl

+        solver = ClpOptimizer(LogLevel = 0)
+        MOIT.contlineartest(solver, linconfig, ["linear10","linear12","linear8a","linear8b","linear8c"])
+    end
+end


You should also add
https://github.com/JuliaOpt/Gurobi.jl/blob/a01f2310ddd83c259fbcf4b7c2512ba1ced4193d/test/MOIWrapper.jl#L65-L85
and
https://github.com/JuliaOpt/Gurobi.jl/blob/odow/moi/test/MOIWrapper.jl#L6-L22
(You may need to exclude some of the tests for quadratic etc.)

Added. Thanks for the review !

odow · 2018-06-14T18:25:25Z

src/MOIWrapper.jl

+"""
+function LQOI.get_linear_objective!(instance::ClpOptimizer, x::Vector{Float64})
+    obj = get_obj_coefficients(instance.inner)
+    for i in 1:length(obj)


copy!(x, obj)

odow · 2018-06-14T18:26:54Z

src/MOIWrapper.jl

+function LQOI.get_linear_constraint(instance::ClpOptimizer, row::Int)::Tuple{Vector{Int}, Vector{Float64}}
+    A = get_constraint_matrix(instance.inner)
+    A_row = A[row,:]
+    return (Array{Int}(A_row.nzind) .- 1, A_row.nzval)


With latest LQOI, we return 1-indexed columns

odow

Good stuff. We should strip out all the commented out functions that relate to MIPs at some point before merging

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

src/MOIWrapper.jl

+    solution = primal_column_solution(instance.inner)
+    for i in 1:length(solution)
+        x[i] = solution[i]
+    end


For all of these, there is copy!(x, solution)

odow · 2018-06-20T03:17:25Z

src/MOIWrapper.jl

+    for i in 1:n
+        numberInColumn = 0
+        rows = Vector{Int32}()
+        elements = Vector{Float64}()


I think Int[] and Float64[] are the preferred style over the above.

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

test/MOIWrapper.jl

+
+    MOIT.basic_constraint_tests(solver, config, exclude = [
+        (MOI.SingleVariable, MOI.Integer),
+        (MOI.SingleVariable, MOI.ZeroOne)        


Hmm you shouldn't have to exclude these. This should only test constraints that you support. (See my comment in supported constraints.)

odow · 2018-06-20T03:18:23Z

src/MOIWrapper.jl

+    (LQOI.SinVar, LQOI.GE),
+    (LQOI.SinVar, LQOI.IV),
+    (LQOI.SinVar, MOI.ZeroOne),
+    (LQOI.SinVar, MOI.Integer),


Clp doesn't support binary and integer constraints

odow · 2018-06-20T03:19:04Z

test/MOIWrapper.jl

+    ])
+
+    MOIT.unittest(solver, config, [
+        "solve_affine_interval", 


Why is this excluded?

odow · 2018-06-30T12:34:07Z

REQUIRE

@@ -1,3 +1,4 @@
 julia 0.6
 Cbc
 MathProgBase 0.5 0.8
+LinQuadOptInterface


This should have a strict bound of LinQuadOptInterface 0.1 0.2

odow · 2018-06-30T12:38:39Z

src/MOIWrapper.jl

+"""
+function LQOI.get_variable_lowerbound(instance::ClpOptimizer, col::Int)::Float64
+    lower = replaceInf(get_col_lower(instance.inner))
+    return lower[col];  


Two comments. Since you only query one element, you don't need to call replaceInf on the whole lowerbound vector. But also, if get_col_lower returns 10+20, why should we change that to Inf? The real bound in Clp is 10+20.

About the second comment. I am honestly about the reason behind that. I just tried to be consistent with the mathprog wrapper. If we fix it we should do it for both at the same time to avoid inconsistencies, especially for those who would like to move from the mathprog wrapper to MOI wrapper. So I suggest we open a separate issue for this.

odow · 2018-06-30T12:38:53Z

src/MOIWrapper.jl

+"""
+function LQOI.get_variable_upperbound(instance::ClpOptimizer, col::Int)::Float64
+    upper = replaceInf(get_col_upper(instance.inner))
+    return upper[col];  


Ditto here. Also trailing ; is unneeded.

odow · 2018-06-30T12:43:50Z

src/MOIWrapper.jl

+    return get_num_rows(instance.inner)
+end
+
+function addrow(row, lower, upper, instance, rows, cols, coefs)


The ordering on these arguments is inconsistent with the rest of the calls. They could be typed for readability?

It is also pretty confusing that you pass in the A matrix but only add one row. At the very least it should be documented

odow · 2018-06-30T12:44:18Z

src/MOIWrapper.jl

+The `sense` is given by `backend_type(m, set)`.
+Ranged constraints (`set=MOI.Interval`) should be added via `add_ranged_constraint!` instead.
+See also: `LinQuadOptInterface.CSRMatrix`.
+"""


Here and throughout: you don't need this docstring since it is already in LQOI

odow · 2018-06-30T12:47:40Z

src/MOIWrapper.jl

+            error("sense must be Cchar(x) where x is in ['L','G',E']")
+        end
+
+        addrow(row, lower, upper, instance, rows, cols, coefs)


I got confused here, see comment with addrow above

I ve added a comment on the function and types for arguments

odow · 2018-06-30T12:49:39Z

src/MOIWrapper.jl

+        upper[row] = coef
+        chg_row_upper(instance.inner, upper)
+    else
+        error("Either row_lower or row_upper must be of abs less than 1e20")


So when adding a constraint it allows bigger coefficients, but it truncates them to 1e20, but when modifying it doesn't? Confusing.

Disallowing for both is needed here. I used it to know the active side of the range

odow · 2018-06-30T12:52:07Z

src/MOIWrapper.jl

+function LQOI.set_linear_objective!(instance::ClpOptimizer, cols::Vector{Int}, coefs::Vector{Float64})::Void
+    obj_in = zeros(Float64, get_num_cols(instance.inner))
+    for i in 1:length(cols)
+        obj_in[cols[i]] = coefs[i]


I think it is sufficient to go obj_in[cols] .= coefs instead of the loop

odow · 2018-06-30T12:53:34Z

src/MOIWrapper.jl

+        numberInColumn = 0
+        rows = Int32[]
+        elements = Float64[]
+        add_column(instance.inner, numberInColumn,  rows, elements, -Inf, +Inf, 0.0)


So Inf bounds are okay here? Not 1e20?

The C code will test and truncate it anyway.

This can probably just be add_column(instance.inner, 0, Int32[], Float64[], -Inf, Inf, 0.0)

IssamT · 2018-07-02T14:13:16Z

The failing test is due to an error inside CLP I think: after deleting a column, getVectorStrats returns a wrong vector. I have reproduced this issue using C interface only and reported it.

https://projects.coin-or.org/Clp/ticket/84

mlubin

The failing test is due to an error inside CLP I think: after deleting a column, getVectorStrats returns a wrong vector. I have reproduced this issue using C interface only and reported it.

That's great, but in my experience there's a 95% chance this bug report will be ignored. The Clp author is more responsive on the mailing list. Disable this test so we can pass on travis and leave a comment explaining the situation.

mlubin · 2018-07-03T10:27:27Z

src/ClpCInterface.jl

+#This function exists in cpp but not c interface
+function add_row(model::ClpModel, number_in_row::Integer, columns::Vector{Int32}, elements::Vector{Float64},
+        row_lower::Float64, row_upper::Float64)
+        _row_starts = Vector{Int32}(2)


Naming: variables in the scope of a function shouldn't start with underscores. _row_upper could be row_upper_vector. Same comment below.

mlubin · 2018-07-03T10:28:21Z

src/MOIWrapper.jl

+using LinQuadOptInterface
+using Clp.ClpCInterface
+
+function replaceInf(x)


naming: no camel case, use replace_inf.

mlubin · 2018-07-03T10:29:08Z

src/MOIWrapper.jl

+end
+
+function ClpOptimizer(;kwargs...)
+    # env = Env()


Style: don't leave code commented without an explanation why. If the code doesn't do anything, remove it.

mlubin · 2018-07-03T10:30:03Z

src/MOIWrapper.jl

+end
+
+"""
+Helper function for adding a row. it adds the row in the components 


Start docstrings with the function signature.

mlubin · 2018-07-03T10:30:20Z

src/MOIWrapper.jl

+Helper function for adding a row. it adds the row in the components 
+that build the LQOI sparse matrix, and it adds the row in the solver.
+"""
+function addrow(row::Int, lower::Float64, upper::Float64, instance::ClpOptimizer, 


Naming: add_row.

mlubin · 2018-07-03T10:30:50Z

src/MOIWrapper.jl

+    cols = A.columns
+    coefs = A.coefficients
+
+    nbrows = length(rhs)


Naming: num_rows.

mlubin · 2018-07-03T10:32:28Z

src/MOIWrapper.jl

+end
+
+"""
+get_linear_constraint(m, row::Int)::Tuple{Vector{Int}, Vector{Float64}}


Formatting: indent the signature with 4 spaces so it displays like code.

mlubin · 2018-07-03T10:33:48Z

src/MOIWrapper.jl

+get_linear_constraint(m, row::Int)::Tuple{Vector{Int}, Vector{Float64}}
+
+Get the linear component of the constraint in the 1-indexed row `row` in
+the model `m`. Returns a tuple of `(cols, vals)`.


Which model m? There's no m below.

mlubin · 2018-07-03T10:34:19Z

src/MOIWrapper.jl

+        return MOI.InfeasibleNoResult
+    elseif s == 2
+        return MOI.UnboundedNoResult
+    # if s in [0, 1, 2] 


Remove or explain why it's commented.

IssamT · 2018-07-09T12:17:20Z

There is an error on Linux but not on Mac. Can someone with Linux check the origin of this error?

blegat · 2018-07-09T13:19:23Z

Yes, linear11 is failing on my Linux computer. You can probably add it in the exclude list. The problem seems to come from Clp and not from the wrapper.
Once I exlude linear11, it passes all the rest of the tests

IssamT · 2018-07-11T15:41:35Z

Thanks @blegat ! We had the same issue with Cbc (since it uses the C++ Clp ), so we disabled the linear11 test there as well.

mlubin · 2018-07-28T01:41:49Z

@IssamT is this ready to merge?

IssamT · 2018-07-28T12:23:04Z

@mlubin, Yes. I've just run the tests with the latest master on MOI / LQOI. (But with julia 0.6)

mlubin · 2018-07-28T12:55:49Z

LQOI 0.2 is released, could you update the PR? It's not good for the ecosystem to start being split by LQOI version.

mlubin · 2018-07-28T13:28:34Z

Is removing the upper bound the right fix? We also don't want the package to break when LQOI is updated. @joaquimg @odow

IssamT · 2018-07-28T14:47:41Z

Sorry @mlubin , but I think I am missing something here.

I have tested the package against MOI master and LQOI master.
However Travis used LQOI 1.0 for the previous commit because the REQUIRE had a version upper bound for LQOI.
I removed LQOI version upper and made this new commit. Hence, travis used LQOI 2.0 to pass the tests.

Which updates of LQOI are you talking about?

mlubin · 2018-07-28T15:01:48Z

LQOI 0.3 could easily contain a change that would break this package. There should be an upper bound on LQOI to prevent users from seeing a broken package, like we do with MOI and MPB dependencies.

mlubin · 2018-07-28T15:35:07Z

The bounds on LQOI look good now. Clp was previously passing tests on 0.7 (https://travis-ci.org/JuliaOpt/Clp.jl/builds/364908514), so the failure looks like a regression. Could you take a quick look at why it's failing?

odow

This isn't ready to merge yet. Mainly because LQOI has been moving forward and not all changes are reflected here (e.g., regarding interval constraints). And there are a few bugs. Let me know if you have time to fix otherwise I'll go through and tidy up.

odow · 2018-07-29T01:10:50Z

test/MOIWrapper.jl

+    config = MOIT.TestConfig()
+    solver = ClpOptimizer(LogLevel = 0)
+    # TODO uncomment after adding MOIT.TestConfig.multiple_bounds
+    # MOIT.basic_constraint_tests(solver, config)


See jump-dev/MathOptInterface.jl#434

odow · 2018-07-29T01:10:58Z

test/MOIWrapper.jl

+    end
+    # TODO uncomment after adding MOIT.TestConfig.multiple_bounds
+    # @testset "orderedindicestest" begin
+    #     MOIT.orderedindicestest(solver)


See jump-dev/MathOptInterface.jl#434

odow · 2018-07-29T01:12:12Z

src/ClpCInterface.jl

+        row_starts_vector[2] = number_in_row
+        row_upper_vector = [row_upper]
+        row_lower_vector = [row_lower]
+        add_rows(model, 1, row_lower_vector, row_upper_vector, row_starts_vector, columns, elements)


Isn't it just as simple to go
add_rows(model, 1, [row_lower], [row_upper], [0, number_in_row], columns, elements)

odow · 2018-07-29T01:12:34Z

src/ClpCInterface.jl

+    _column_starts[2] = number_in_column
+    _column_upper = [column_upper]
+    _column_lower = [column_lower]
+    _objective = [objective]


Ditto here, or at least no underscore variable names

odow · 2018-07-29T01:14:40Z

src/MOIWrapper.jl

+            lowerbounds[cols[i]] = values[i]
+        else            
+            error("sense is " * string(senses[i]) * ", but only " * Cchar('U') *  
+            " and " * Cchar('L') * " senses are supported")


Is this preferred over string interpolation?
"sense is $(senses[i]), but only 'U' and 'L' senses are supported."

odow · 2018-07-29T01:33:00Z

src/MOIWrapper.jl

+            error("Either row_lower or row_upper must be of abs less than 1e20")
+        end
+
+        sense = sense[i]


Bug? Is this tested? I shouldn't imagine that this passed...

odow · 2018-07-29T01:34:03Z

src/MOIWrapper.jl

+"""
+function LQOI.set_linear_objective!(instance::ClpOptimizer, cols::Vector{Int}, coefs::Vector{Float64})::Void
+    obj_in = zeros(Float64, get_num_cols(instance.inner))
+    obj_in[cols] .= coefs


Doesn't account for duplicates. See jump-dev/MathOptInterface.jl#432

odow · 2018-07-29T01:35:41Z

src/MOIWrapper.jl

+        numberInColumn = 0
+        rows = Int32[]
+        elements = Float64[]
+        add_column(instance.inner, numberInColumn,  rows, elements, -Inf, +Inf, 0.0)


This can probably just be add_column(instance.inner, 0, Int32[], Float64[], -Inf, Inf, 0.0)

odow · 2018-07-29T01:39:00Z

src/MOIWrapper.jl

+        return lb
+    elseif ub < Inf
+        return ub
+    else


What about interval constraints? You need to implement get_range

And also modify_ranged_constraints!

odow · 2018-07-29T01:41:30Z

test/MOIWrapper.jl

+        # linear1 test is disabled due to the following bug.
+        # https://projects.coin-or.org/Clp/ticket/84
+        MOIT.contlineartest(solver, linconfig, ["linear1", "linear10","linear11",
+            "linear12","linear8a","linear8b","linear8c"])


Why are these tests excluded?

more tests.

odow · 2018-07-29T05:20:39Z

@IssamT there is a PR with some changes: IssamT#1

IssamT · 2018-07-30T14:33:53Z

@mlubin, looking quickly at the error on 0.7, apparently it is due to LQOI failing under 0.7, which is due to Nullables.jl failing under 0.7.

Supporting LQOI on both 0.6, 0.7 requires Nullables.

joaquimg · 2018-07-30T16:38:59Z

Do you know why we need nullables in LQOI?
I think its always good to avoid dependencies, but if there is no other way lets go for it.

IssamT · 2018-07-30T17:22:32Z

Nullable is disappearing in julia 0.7 in favor of Union{T, Nothing}. Nullables.jl offer a generic way of using Nullables that hides the difference between 0.6 and 0.7 (see JuliaLang/julia#23642)

However, as far as I can tell, Nullable is only used for single_obj_var::Nullable{MOI.VariableIndex}

I am not sure if that justifies a dependency ...

Tidy up

odow · 2018-08-02T22:28:14Z

Okay, Nullables was removed from LQOI. Now we just need to wait for MOI to be tagged, then LQOI, then we can merge this with a new bound for LQOI.

We should also refactor ClpOptimizer to Clp.Optimizer ala GLPK and Gurobi.

codecov-io · 2018-08-09T21:46:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a7c6223). Click here to learn what that means.
The diff coverage is 72.56%.

@@            Coverage Diff            @@
##             master      #27   +/-   ##
=========================================
  Coverage          ?   48.13%           
=========================================
  Files             ?        3           
  Lines             ?      644           
  Branches          ?        0           
=========================================
  Hits              ?      310           
  Misses            ?      334           
  Partials          ?        0

Impacted Files	Coverage Δ
src/ClpCInterface.jl	`34.62% <100%> (ø)`
src/MOIWrapper.jl	`72.22% <72.22%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7c6223...8806d61. Read the comment docs.

odow · 2018-08-09T22:25:49Z

You just need to add 0.7 on travis and make sure it passes

odow · 2018-08-09T22:26:49Z

test/MOIWrapper.jl

+@testset "Unit Tests" begin
+    config = MOIT.TestConfig()
+    solver = Clp.Optimizer(LogLevel = 0)
+    # MOIT.basic_constraint_tests(solver, config)


This can be enabled

IssamT · 2018-08-14T09:17:37Z

Is there still something required for this PR?

odow · 2018-08-14T21:42:27Z

There a lot of deprecation warnings, but they can be addressed in another PR.

IssamT added 4 commits May 29, 2018 12:48

MOI interface through LQOI (Partially implemented)

b9c81fe

more impl.

0a07fdb

Finished first impl and fixed tests.

553b9ea

updated to use CSR struct

adf7810

odow reviewed Jun 14, 2018

View reviewed changes

minor updates. more tests

35b982c

odow reviewed Jun 20, 2018

View reviewed changes

IssamT added 3 commits June 20, 2018 11:56

a few fixes

da2ad75

using copy!

9d84bcc

removed some commented out code

1c60f52

odow reviewed Jun 30, 2018

View reviewed changes

minor modifs

d4042f0

mlubin reviewed Jul 3, 2018

View reviewed changes

some fixes

5dfb617

exculded test linear11

a79e3fd

removed the LQOI version upperbound

b86374b

limited LQOI to 2.0 inclusive

a89e1c3

odow requested changes Jul 29, 2018

View reviewed changes

Tidy up. Remove unneeded docstrings, fix a few functions, enable

ea529ee

more tests.

odow added 4 commits July 29, 2018 20:33

Handle infeasiblity certificates and other matters:

9aca0f6

Minor re-org

07d4e91

Remove unneeded code

3728aaf

Exclude linear11 test

0c8fc6d

joaquimg mentioned this pull request Jul 30, 2018

Remove or Add Nullables dependency JuliaOpt/LinQuadOptInterface.jl#42

Closed

Merge pull request #1 from JuliaOpt/moi

6957e3c

Tidy up

odow mentioned this pull request Aug 1, 2018

Help Wanted: Update for MOI jump-dev/CPLEX.jl#180

Closed

minor updates

af551f7

IssamT added 2 commits August 9, 2018 23:48

renaming ClpOptimizer --> Clp.Optimizer

575aff7

update requires

4848bbe

odow approved these changes Aug 9, 2018

View reviewed changes

odow reviewed Aug 9, 2018

View reviewed changes

enabled a test, added 0.7 to travis

8806d61

odow merged commit afc14ee into jump-dev:master Aug 14, 2018

WIP : MOI wrapper (based on LQOI) #27

WIP : MOI wrapper (based on LQOI) #27

Conversation

IssamT commented May 29, 2018

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

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

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 Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IssamT commented Jul 2, 2018

mlubin left a comment • edited Loading

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 Jul 9, 2018

blegat commented Jul 9, 2018 • edited Loading

IssamT commented Jul 11, 2018

mlubin commented Jul 28, 2018

IssamT commented Jul 28, 2018

mlubin commented Jul 28, 2018

mlubin commented Jul 28, 2018

IssamT commented Jul 28, 2018 • edited Loading

mlubin commented Jul 28, 2018

mlubin commented Jul 28, 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

odow commented Jul 29, 2018

IssamT commented Jul 30, 2018

joaquimg commented Jul 30, 2018

IssamT commented Jul 30, 2018

odow commented Aug 2, 2018 • edited Loading

codecov-io commented Aug 9, 2018 • edited Loading

Codecov Report

odow commented Aug 9, 2018

Choose a reason for hiding this comment

IssamT commented Aug 14, 2018

odow commented Aug 14, 2018

IssamT Jun 30, 2018 •

edited

Loading

mlubin left a comment •

edited

Loading

blegat commented Jul 9, 2018 •

edited

Loading

IssamT commented Jul 28, 2018 •

edited

Loading

odow commented Aug 2, 2018 •

edited

Loading

codecov-io commented Aug 9, 2018 •

edited

Loading