MatrixOfConstraints #1287

blegat · 2021-03-14T22:40:05Z

Current benchmark:

import MathOptInterface
const MOI = MathOptInterface
const MOIU = MOI.Utilities
import Clp
function bench(n, m)
    model = MOIU.Model{Float64}()
    x = MOI.add_variables(model, n)
    fx = MOI.SingleVariable.(x)
    ofx = 1.0 .* fx
    for i in 1:n
        MOI.add_constraint(model, ofx[i], MOI.LessThan(1.0))
        MOI.add_constraint(model, ofx[i], MOI.GreaterThan(1.0))
    end
    s = sum(ofx)
    for i in 1:n
        MOI.add_constraint(model, s, MOI.EqualTo(1.0))
    end
    optimizer = MOI.Bridges.full_bridge_optimizer(
        MOIU.CachingOptimizer(
            M(),
            Clp.Optimizer(),
        ),
        Float64,
    )
    @time MOI.copy_to(optimizer, model)
    @time MOIU.attach_optimizer(optimizer.model)
    optimizer.model
end

With this branch, Cbc#od/moi10 and M=MOIU.Model{Float64}, I get:

julia> bench(1000, 1000);
  0.035744 seconds (18.71 k allocations: 32.124 MiB)
  0.075348 seconds (429 allocations: 42.859 MiB)

With this branch, Cbc#bl/matrix and M=Cbc.Model, I get:

julia> bench(1000, 1000);
  0.011251 seconds (698 allocations: 551.617 KiB)
  0.000164 seconds (105 allocations: 146.023 KiB)

What really matters is the sum of the two. The speedup is approximately 10x.
~~We already have a significant decrease but I think we could have a number of allocation which does not scale linearly with n and m so I'm trying to figure it out.~~
The number of allocations is a constant + some logarithmic on n and m which is expected (e.g. because of push in caches).

See #1261

Requires #1267, #1286

odow · 2021-04-19T04:27:48Z

What's the status of this? Can we make a new MOI release without it?

blegat · 2021-04-19T20:34:48Z

There is still some work needed, we don't need to wait for it for a new MOI release

src/Test/config.jl

odow · 2021-05-09T22:44:41Z

As another benchmark: how is the first solve? Compilation times are our big killer for this.

odow

This is quite hard to review because there is a lot of new types being added

ProductOfSets
MixOfScalarSets
OrderedProductOfSets
OrderedProductOfScalarSets
UnevenIterator
Box
MatrixOfConstraints
IdentityMap
MutableSparseMatrixCSC

Can we break it down into smaller pieces? It's not obvious why we need so many different structures, or how they inter-relate.

src/Utilities/copy.jl

odow · 2021-05-10T19:47:10Z

src/Utilities/copy.jl

    single_variable_not_added
 end

+function final_touch(::MOI.ModelLike, idxmap) end


This needs a docstring.

Let me know what you think for the name. Another option would be end_of_copy

src/Utilities/matrix_of_constraints.jl

odow · 2021-05-10T19:50:43Z

src/Utilities/model.jl

    if model.variable_indices !== nothing
        push!(model.variable_indices, vi)
    end
+    _add_variable(model.constraints)


It seems weird to force other types to overload _add_variable. Why not just overload MOI.add_variable(model.constraints)?

Then VectorOfConstraints should return a VariableIndex but currently it does not keep track of this information.

odow

This needs documentation added to the Utilities part of the manual.

I wonder about making a subdirectory in src/Utilities to contain these. That would isolate the code a little more, and better document what goes with what.

I'm also keen to see what impact this has on our time-to-first-solve issue.

odow · 2021-05-14T04:15:16Z

test/Utilities/copy.jl

        return new{F,S}(MOIU.VectorOfConstraints{F,S}())
    end
 end
+function MOI.Utilities._add_variable(::OnlyCopyConstraints) end


If this is a public API that people need to implement, it shouldn't begin with _.

This needs to be implemented by the type of the constraints field but it's currently not public API.
It's best we don't let user create new types at the moment until we converge. This is similar to _delete_variables, _deleted_constraints and _throw_if_cannot_delete.

src/Utilities/matrix_of_constraints.jl

odow · 2021-05-14T04:27:02Z

src/Utilities/matrix_of_constraints.jl

+end
+
+function final_touch(model::MatrixOfConstraints, index_map)
+    num_rows = number_of_rows(model.sets)


This needs documenting

odow · 2021-05-14T04:27:09Z

src/Utilities/matrix_of_constraints.jl

+function final_touch(model::MatrixOfConstraints, index_map)
+    num_rows = number_of_rows(model.sets)
+    resize!(model.constants, num_rows)
+    set_number_of_rows(model.coefficients, num_rows)


This needs documenting

src/Utilities/matrix_of_constraints.jl

odow · 2021-05-14T04:29:20Z

src/Utilities/product_of_sets.jl

+Return an integer corresponding to the index of the set type in the list given
+by [`set_types`](@ref)..
+"""
+set_index(sets::ProductOfSets, ::Type{S}) where {S<:MOI.AbstractSet} = nothing


This needs to be documented in MatrixOfConstraints

src/Utilities/sparse_matrix.jl

odow

I have some reservations about adding yet more complexity to MOI. And it would be better if the documentation clearly identified which functions needed to be implemented for which bits, but it may be easier to merge and keep iterating in new PRs. We still have time to break things, so we should merge and then update solvers in branches to figure out what tweaks need to be made.

odow · 2021-05-15T03:32:24Z

src/Utilities/product_of_sets.jl

+    type_def = :(struct $esc_name{$T} <: $MOIU.OrderedProductOfScalarSets{$T}
+        num_rows::Vector{Int}
+        function $esc_name{$T}() where {$T}
+            return new(zeros(Int, $(1 + length(set_types))))


So all this is a OrderedProductOfSets but without the dimension field? Then the only efficiency is that we save a Dict{Int,Int}() call, and a call on empty!?

Yes, that not so much. I used to store the dimension for the scalar sets for OrderedProductOfSets so OrderedProductOfScalarSets was a big win but now even OrderedProductOfSets don't store the dimension of scalar sets so we could indeed get rid of OrderedProductOfScalarSets (although it's just a few lines of code).

Part of this is that it's just a few lines of code, but much more code to compile.

Yes, I think we can remove it

odow · 2021-05-15T03:34:36Z

docs/src/submodules/Utilities/reference.md

+
+```@docs
+Utilities.ProductOfSets
+Utilities.set_index


These functions (set_index, set_types, add_set, indices) get called by MatrixOfConstraints, so they're part of that API. ProductOfSets is just one implementation of the .sets field.

Is any solver going to implement a different type for the .sets field? I wonder if we're making our life harder by making this design too flexible...

I made this into a well-defined API to make the code more understandable. We will need yet another one for list of sets that are not known at compile time, e.g., DiffOpt but we might have enough with 3, can't say for now.

blegat · 2021-05-18T03:51:29Z

And it would be better if the documentation clearly identified which functions needed to be implemented for which bits, but it may be easier to merge and keep iterating in new PRs. We still have time to break things, so we should merge and then update solvers in branches to figure out what tweaks need to be made.

Yes I agree. It's ready so that people can start play with it and we can iterate a bit before releasing.

blegat force-pushed the bl/pass_cons branch 2 times, most recently from dd03250 to 6206427 Compare March 21, 2021 16:35

blegat mentioned this pull request Mar 21, 2021

Pass copy operation for non-variable constraints #1286

Merged

3 tasks

Base automatically changed from bl/pass_cons to master March 21, 2021 21:38

blegat force-pushed the bl/matrix_of_constraints branch from 1a42cb5 to 9b3166b Compare March 21, 2021 23:19

blegat mentioned this pull request Mar 21, 2021

Replace MatOI with MOI jump-dev/DiffOpt.jl#84

Closed

blegat marked this pull request as ready for review March 22, 2021 21:17

blegat mentioned this pull request Apr 7, 2021

Use matrix of constraints jump-dev/Clp.jl#111

Closed

blegat force-pushed the bl/matrix_of_constraints branch from 235c46a to 3565b31 Compare May 2, 2021 17:20

blegat mentioned this pull request May 3, 2021

Add auto_bridge to CachingOptimizer #1252

Closed

7 tasks

blegat force-pushed the bl/matrix_of_constraints branch 2 times, most recently from 8cf4d33 to ea094b7 Compare May 4, 2021 07:14

blegat mentioned this pull request May 5, 2021

Remove useless {S} #1318

Merged

blegat force-pushed the bl/matrix_of_constraints branch from ea094b7 to 38da881 Compare May 9, 2021 07:28

blegat mentioned this pull request May 9, 2021

inbounds for is_strictly_increasing #1325

Merged

odow reviewed May 9, 2021

View reviewed changes

src/Test/config.jl Show resolved Hide resolved

blegat force-pushed the bl/matrix_of_constraints branch 2 times, most recently from f3a2afb to 866bca9 Compare May 9, 2021 22:48

blegat mentioned this pull request May 9, 2021

Add query_number_of_constraints option #1327

Merged

blegat force-pushed the bl/matrix_of_constraints branch 3 times, most recently from c875104 to 5edfaaf Compare May 10, 2021 11:38

blegat mentioned this pull request May 10, 2021

Add throw_if_scalar_and_constant_not_zero #1330

Merged

odow reviewed May 10, 2021

View reviewed changes

blegat force-pushed the bl/matrix_of_constraints branch from 5edfaaf to 71d0a8b Compare May 12, 2021 10:52

blegat mentioned this pull request May 12, 2021

Deprecate Allocate-Load #1341

Closed

MatrixOfConstraints

89ed26a

Refactor with product_of_sets.jl and add docstrings

82e94cd

blegat force-pushed the bl/matrix_of_constraints branch from aa37bf9 to 82e94cd Compare May 14, 2021 04:08

odow reviewed May 14, 2021

View reviewed changes

blegat force-pushed the bl/matrix_of_constraints branch from 40ced0d to e806115 Compare May 14, 2021 15:39

Add documentation and tests

b629e45

blegat force-pushed the bl/matrix_of_constraints branch from e806115 to b629e45 Compare May 14, 2021 19:08

blegat mentioned this pull request May 15, 2021

set field of CleverDict #1326

Closed

blegat requested a review from odow May 18, 2021 00:10

odow approved these changes May 18, 2021

View reviewed changes

blegat merged commit 7006a17 into master May 18, 2021

blegat deleted the bl/matrix_of_constraints branch May 18, 2021 03:51

blegat added this to the v0.10 milestone May 22, 2021

MatrixOfConstraints #1287

MatrixOfConstraints #1287

Uh oh!

Conversation

blegat commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odow commented Apr 19, 2021

Uh oh!

blegat commented Apr 19, 2021

Uh oh!

Uh oh!

odow commented May 9, 2021

Uh oh!

odow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

blegat commented May 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

blegat commented Mar 14, 2021 •

edited

Loading