-
Notifications
You must be signed in to change notification settings - Fork 120
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
Proof of concept more-MOIified implementation #393
Conversation
src/solve2!.jl
Outdated
model = MOIB.full_bridge_optimizer(MOIU.CachingOptimizer(MOIU.UniversalFallback(MOIU.Model{T}()), | ||
optimizer), T) | ||
|
||
return (var_id_to_moi_indices=OrderedDict{UInt64,Vector{MOI.VariableIndex}}(), |
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.
this could be a struct
instead of a NamedTuple
, but for prototyping I think it's easier as a NamedTuple
src/solve2!.jl
Outdated
scalar_fn(x) = only(MOIU.scalarize(x)) | ||
scalar_fn(v::MOI.AbstractScalarFunction) = v |
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'm not sure this should really be needed. It came about because I ended up passing things that were nomially vectors to the objective function, or adding scalars and vectors. I think part of the problem is that Convex.jl does not distinguish between scalars and vectors (or matrices!); e.g. atoms are not parametric types. This is an inherent source of type instability and means that we can't use dispatch always to route things correctly (i.e. one might imagine that the objective function would always end up as a scalar function just by the layout of the problem, but that doesn't happen currently).
src/solve2!.jl
Outdated
function solve2!(problem::Problem{T}, optimizer; kwargs...) where {T} | ||
if Base.applicable(optimizer) | ||
return solve!(problem, optimizer(); kwargs...) | ||
else | ||
throw(ArgumentError("MathProgBase solvers like `solve!(problem, SCSSolver())` are no longer supported. Use instead e.g. `solve!(problem, SCS.Optimizer)`.")) | ||
end | ||
end |
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.
copied from solve!
if primal_status != MOI.NO_SOLUTION | ||
for (id, var_indices) in var_to_indices | ||
var = id_to_variables[id] | ||
vexity(var) == ConstVexity() && continue |
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.
this means a variable has been fix!
'd and should be treated as a constant; in particular, we should not update its value
src/variable_conic_form.jl
Outdated
|
||
function template(a::AbstractVariable, context) | ||
var_inds = get!(context.var_id_to_moi_indices, a.id_hash) do | ||
return add_variables!(context.model, a::Variable) |
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.
there are two leaf types: Constant
s and AbstractVariable
s. When we reach a variable, we add it to our local state (in context
) by adding it to the MOI model and to our dictionary. I've used Convex.jl's convention of an explicit hash field, but I think a usual dictionary that does its own hashing might be cleaner.
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.
Comment doesn't make sense as written - did you mean "does its own hashing"?
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.
yep, sorry about that. By the way, I am unsure what to do with the hashing stuff in Convex; it's used to avoid reformulating branches of the expression tree if they are identical and have already been formulated, but I don't entirely understand why it works (sometimes the head
symbol of an atom is hashed in, sometimes it's not; seems like it should be a source of bugs but it hasn't been yet...) and think it should be revamped.
In this PR, only the id_hash
field of an AbstractVariable is used to recognize the same variable in different branches of the expression tree, etc, but no other hashes are used. So in particular, in this PR currently identical branches are reformulated multiple times.
@@ -0,0 +1,126 @@ | |||
using ECOS |
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.
This problem is from #390
src/constraints/constraints.jl
Outdated
@@ -154,6 +154,16 @@ function conic_form!(c::GtConstraint, unique_conic_forms::UniqueConicForms) | |||
return get_conic_form(unique_conic_forms, c) | |||
end | |||
|
|||
function add_constraints_to_context(lt::GtConstraint, context) |
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've only implemented GtConstraint
so far, but the idea is to have such a method for each Convex.Constraint
(like conic_form!
). I did not use template
because I think constraints should not return an objective function value but instead simply update the model (by adding constraints).
src/atoms/second_order_cone/norm2.jl
Outdated
@@ -48,6 +48,29 @@ function conic_form!(x::EucNormAtom, unique_conic_forms::UniqueConicForms) | |||
return get_conic_form(unique_conic_forms, x) | |||
end | |||
|
|||
|
|||
function template(A::EucNormAtom, context) |
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.
This seems like a case where a partially specified problem might come in handy
var = var * ones(1, size(coeff, 1)) | ||
end | ||
|
||
const_multiplier = Diagonal(vec(coeff)) |
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 copied this method from the conic_form!
one (but using Diagonal
instead of spdiagm
, which I thought might be more efficient here?). Convex.jl uses a lot of these "vectorized" methods where we create a big matrix to act on the vector of variables. I wonder if there are better ways that don't involve constructing giant matrices.
Codecov Report
@@ Coverage Diff @@
## master #393 +/- ##
==========================================
- Coverage 88.42% 84.04% -4.39%
==========================================
Files 75 77 +2
Lines 3508 3691 +183
==========================================
Hits 3102 3102
- Misses 406 589 +183
Continue to review full report at Codecov.
|
In this PR, I introduce a structure struct VectorAffineFunctionAsMatrix{M,B,V}
matrix::M
vector::B
variables::V
end to represent an However, the final matrix should be |
I've pushed a commit that implements the ideas from the previous comment (and then one that fixes perf bugs), which improves the benchmarks quite a bit: julia> include("testproblem\\testproblem.jl")
┌ Info: Running with classic `Convex.solve!`...
└ (MAX_ITERS, m, n, k) = (2, 150, 150, 5)
15.400637 seconds (1.40 M allocations: 10.077 GiB, 14.19% gc time)
┌ Info: Running with `Convex.solve2!`...
└ (MAX_ITERS, m, n, k) = (2, 150, 150, 5)
2.386677 seconds (121.63 k allocations: 785.039 MiB, 6.23% gc time)
┌ Info: Running with JuMP...
└ (MAX_ITERS, m, n, k) = (2, 150, 150, 5)
5.013906 seconds (15.27 M allocations: 1.047 GiB, 3.48% gc time) and julia> include("testproblem\\testproblem.jl")
┌ Info: Running with `Convex.solve2!`...
└ (MAX_ITERS, m, n, k) = (2, 300, 300, 5)
16.237871 seconds (148.79 k allocations: 4.973 GiB, 5.62% gc time)
┌ Info: Running with JuMP...
└ (MAX_ITERS, m, n, k) = (2, 300, 300, 5)
29.201845 seconds (55.55 M allocations: 3.898 GiB, 8.59% gc time) I could even solve it with julia> include("testproblem\\testproblem.jl")
┌ Info: Running with `Convex.solve2!`...
└ (MAX_ITERS, m, n, k) = (1, 500, 500, 5)
43.958516 seconds (133.30 k allocations: 10.646 GiB, 4.62% gc time) (note that I set One thing to note is that the performance of this approach is sensitive to missing specialized dispatches. |
To achieve type stability with this approach (or most others), we will need to make atoms parametric on the number of dimensions, because we often have different implementations for vectors vs scalars vs matrices. Here, the implementation leads to a choice of One other thought is that the |
The latest commit adds the unregistered dependency https://github.com/ericphanson/MatrixChainMultiply.jl to look at the impact of using that method for choosing how to evaluate the |
I'm starting to see how this can all work together. This rewrite is actually not very different than how Convex.jl works now, but I think will actually end up a lot simpler. The idea is the following (note this is mostly for myself to put into the dev docs at some point..): There are two "levels" at which Convex operates: the high-level that users use, and the MOI/
As a design point, I will try to do as much as possible at the high-level, since this makes the package most accessible to contributions from Convex.jl users, and simplifies the codebase. The functions many atoms implement do not actually need to be represented by atoms; e.g. I've already removed the In order to make use of more MOI features, however, (e.g. another cone like for vector A note on |
I’m gonna leave this for now; I had it timeboxed to a week and couldn’t push it through but I think the idea works (the idea is very simple, mostly following the existing structure; just recurse through the expression tree, lazily building the affine transformations to the variables and eagerly adding constraints to the model. That being said this is the ~4th reimplementation I’ve written code for, so somehow it wasn’t obvious to me from the start). What’s left is just more implementation and some fixes (there’s some test failures that indicate I’ve made some mistakes), and cleanup to remove all the old code. Hopefully I or someone else can pick this up at some point soon! Also, I have tried out four slightly different ways of holding the tape of affine operations; I think |
Just had a stray thought for how to do the hashing for deeply nested trees that repeat themselves. Currently we can have hash collisions (#78) and the hashing step can be a performance bottleneck (there is a discourse post somewhere). In this PR I removed it / didn’t implement it for the new way because of these issues. But I think we could do something simple like every atom is a mutable struct (so they are semantically distinguishable) and we cache their templates via an |
I haven't looked in detail at what is happening here yet but I saw you mentioned complex variables in the initial post, and there now exists https://github.com/jump-dev/ComplexOptInterface.jl. |
I did see that, but it was/is still pretty early for that effort. My thought in this PR was to just continue supporting complex variables for the current atoms by reformulating as we already doing but that future work might be able to involve that.
No recent ones, but to summarize my thoughts on this:
Also, unfortunately I probably won't be working much on this stuff for the forseeable future, but I'm happy to share thoughts / knowledge of convex internals with anyone interested in working on it. |
closing in favor of #504 |
This is a proof of concept implementation of a different internal structure for Convex.jl (edit: which has started to turn into a bit of a rewrite).
Currently, just the minimal amount of methods are implemented to solve
testproblem/testproblem.jl
. Probably any other problem will error! The end-user access point issolve2!
which works just likesolve!
(except much of it isn't implemented).Some design notes
template
basically has the same motivation asconic_form!
: it recurses through the problem, generating a conic reformulation of it. It does so in a different way, however. It takes acontext
parameter which acts very similarly to howunique_conic_forms
currently works by holding problem-local state. In this case, it holds a MOI model. When constructing the conic reformulation, any Convex AbstractVariables are associated to MOI variables added to the model. Any constraints are simply immediately added to the model. Then a MOI function is returned. In the end, we recover an MOI function which is the objective function for the problem.This implementation reuses the current atoms and DCP properties, but the conic reformulations are entirely rewritten (
template
instead ofconic_form!
) which means that all the objects inconic_form.jl
andsolution.jl
are not used.Some more notes:
template
on the children. We should think about when to use one versus the other.!
when modifying thecontext
object, under the assumption that it is often being modified.On the problem in
testproblem/testproblem.jl
, we get much improved results:It should also be much easier to support other cones to support e.g.
geomean
for vectors (ref #388).To do in order to think about merging this
ComplexVariable
,ComplexConstant
, andComplexTape
types)satisfy
LtConstraint
,GtConstraint
,EqConstraint
,SDPConstraint
,ExponentialCone
)New features that this enables
logdet
to the MOI cone, enabling Hypatia to use the native cone and MOI bridges otherwise