Merge pull request #322 from ericphanson/remove-id-to-variables

Remove global variables

Author: ericphanson

src/Convex.jl

@@ -9,8 +9,11 @@ global DEFAULT_SOLVER = nothing
 ### modeling framework
 include("dcp.jl")
 include("expressions.jl")
-include("conic_form.jl")
+# need to define `Variable` before `UniqueConicForms`
 include("variable.jl")
+include("conic_form.jl")
+# need to define `conic_form!` for `Variable`s after `UniqueConicForms`
+include("variable_conic_form.jl")
 include("constant.jl")
 include("constraints/constraints.jl")
 include("constraints/signs_and_sets.jl")
@@ -85,13 +88,9 @@ include("utilities/show.jl")
 include("utilities/iteration.jl")
 include("utilities/broadcast.jl")
 
-#Temporary workaround for memory leak (https://github.com/JuliaOpt/Convex.jl/issues/83)
+# Deprecated workaround for memory leak (https://github.com/JuliaOpt/Convex.jl/issues/83)
 function clearmemory()
-    global id_to_variables
-    empty!(id_to_variables)
-    global conic_constr_to_constr 
-    empty!(conic_constr_to_constr)
-    GC.gc()
+    Base.depwarn("Convex.clearmemory() is deprecated, as the memory leak it works around has been closed (in https://github.com/JuliaOpt/Convex.jl/pull/322). This function no longer does anything and will be removed in a future Convex.jl release.", :clearmemory )
 end
 
 end

src/atoms/affine/stack.jl

@@ -54,7 +54,7 @@ function conic_form!(x::HcatAtom, unique_conic_forms::UniqueConicForms=UniqueCon
                     if id == objectid(:constant)
                         variable_to_sizes[id] = 1
                     else
-                        variable_to_sizes[id] = length(id_to_variables[id])
+                        variable_to_sizes[id] = length(unique_conic_forms.id_to_variables[id])
                     end
                 end
             end

src/conic_form.jl

@@ -106,15 +106,19 @@ const UniqueExpMap = OrderedDict{Tuple{Symbol, UInt64}, ConicObj}
 const UniqueConstrMap = OrderedDict{Tuple{Symbol, UInt64}, Int}
 # records each ConicConstr created
 const UniqueConstrList = Vector{ConicConstr}
-
+# map variables' hash to the variable itself 
+const IdToVariables = OrderedDict{UInt64, Variable}
+const ConicConstrToConstr = Dict{ConicConstr, Constraint}
 # UniqueConicForms caches all the conic forms of expressions we've parsed so far
 struct UniqueConicForms
     exp_map::UniqueExpMap
     constr_map::UniqueConstrMap
     constr_list::UniqueConstrList
+    id_to_variables::IdToVariables
+    conic_constr_to_constr::ConicConstrToConstr
 end
 
-UniqueConicForms() = UniqueConicForms(UniqueExpMap(), UniqueConstrMap(), ConicConstr[])
+UniqueConicForms() = UniqueConicForms(UniqueExpMap(), UniqueConstrMap(), ConicConstr[], IdToVariables(), ConicConstrToConstr())
 
 function has_conic_form(conic_forms::UniqueConicForms, exp::AbstractExpr)
     return haskey(conic_forms.exp_map, (exp.head, exp.id_hash))
@@ -145,3 +149,7 @@ function cache_conic_form!(conic_forms::UniqueConicForms, constr::Constraint, ne
     conic_forms.constr_map[(constr.head, constr.id_hash)] = 0
     append!(conic_forms.constr_list, new_conic_forms)
 end
+
+function add_to_id_to_variables!(conic_forms::UniqueConicForms, var::Variable)
+    conic_forms.id_to_variables[var.id_hash] = var
+end

src/constraints/constraints.jl

@@ -2,8 +2,6 @@ import Base.==, Base.<=, Base.>=, Base.<, Base.>
 export EqConstraint, LtConstraint, GtConstraint
 export ==, <=, >=
 
-const conic_constr_to_constr = Dict{ConicConstr, Constraint}()
-
 ### Linear equality constraint
 mutable struct EqConstraint <: Constraint
     head::Symbol
@@ -42,14 +40,14 @@ function conic_form!(c::EqConstraint, unique_conic_forms::UniqueConicForms=Uniqu
             expr = c.lhs - c.rhs
             objective = conic_form!(expr, unique_conic_forms)
             new_constraint = ConicConstr([objective], :Zero, [c.size[1] * c.size[2]])
-            conic_constr_to_constr[new_constraint] = c
+            unique_conic_forms.conic_constr_to_constr[new_constraint] = c
         else
             real_expr = real(c.lhs - c.rhs)
             imag_expr = imag(c.lhs - c.rhs)
             real_objective = conic_form!(real_expr, unique_conic_forms)
             imag_objective = conic_form!(imag_expr, unique_conic_forms)
             new_constraint = ConicConstr([real_objective, imag_objective], :Zero, [c.size[1] * c.size[2], c.size[1] * c.size[2]])
-            conic_constr_to_constr[new_constraint] = c
+            unique_conic_forms.conic_constr_to_constr[new_constraint] = c
         end
         cache_conic_form!(unique_conic_forms, c, new_constraint)
     end
@@ -100,7 +98,7 @@ function conic_form!(c::LtConstraint, unique_conic_forms::UniqueConicForms=Uniqu
         expr = c.rhs - c.lhs
         objective = conic_form!(expr, unique_conic_forms)
         new_constraint = ConicConstr([objective], :NonNeg, [c.size[1] * c.size[2]])
-        conic_constr_to_constr[new_constraint] = c
+        unique_conic_forms.conic_constr_to_constr[new_constraint] = c
         cache_conic_form!(unique_conic_forms, c, new_constraint)
     end
     return get_conic_form(unique_conic_forms, c)
@@ -152,7 +150,7 @@ function conic_form!(c::GtConstraint, unique_conic_forms::UniqueConicForms=Uniqu
         expr = c.lhs - c.rhs
         objective = conic_form!(expr, unique_conic_forms)
         new_constraint = ConicConstr([objective], :NonNeg, [c.size[1] * c.size[2]])
-        conic_constr_to_constr[new_constraint] = c
+        unique_conic_forms.conic_constr_to_constr[new_constraint] = c
         cache_conic_form!(unique_conic_forms, c, new_constraint)
     end
     return get_conic_form(unique_conic_forms, c)

src/problems.jl

@@ -52,7 +52,7 @@ end
 # constr_size: m
 # var_to_ranges a dictionary mapping from variable id to (start_index, end_index)
 # where start_index and end_index are the start and end indexes of the variable in A
-function find_variable_ranges(constraints)
+function find_variable_ranges(constraints, id_to_variables)
     index = 0
     constr_size = 0
     var_to_ranges = Dict{UInt64, Tuple{Int, Int}}()
@@ -127,10 +127,13 @@ function conic_problem(p::Problem)
     unique_conic_forms = UniqueConicForms()
     objective, objective_var_id = conic_form!(p, unique_conic_forms)
     constraints = unique_conic_forms.constr_list
+    conic_constr_to_constr = unique_conic_forms.conic_constr_to_constr
+    id_to_variables = unique_conic_forms.id_to_variables
+
     # var_to_ranges maps from variable id to the (start_index, stop_index) pairs of the columns of A corresponding to that variable
     # var_size is the sum of the lengths of all variables in the problem
     # constr_size is the sum of the lengths of all constraints in the problem
-    var_size, constr_size, var_to_ranges = find_variable_ranges(constraints)
+    var_size, constr_size, var_to_ranges = find_variable_ranges(constraints, id_to_variables)
     c = spzeros(var_size, 1)
     objective_range = var_to_ranges[objective_var_id]
     c[objective_range[1]:objective_range[2]] .= 1
@@ -188,7 +191,7 @@ function conic_problem(p::Problem)
         c = -c
     end
 
-    return c, A, b, cones, var_to_ranges, vartypes, constraints
+    return c, A, b, cones, var_to_ranges, vartypes, constraints, id_to_variables, conic_constr_to_constr
 end
 
 Problem(head::Symbol, objective::AbstractExpr, constraints::Constraint...) =

src/solution.jl

@@ -33,20 +33,20 @@ function solve!(problem::Problem;
         vex = vexity(problem)
     end
 
-    c, A, b, cones, var_to_ranges, vartypes, conic_constraints = conic_problem(problem)
+    c, A, b, cones, var_to_ranges, vartypes, conic_constraints, id_to_variables, conic_constr_to_constr = conic_problem(problem)
 
     # load MPB conic problem
     m = problem.model
     load_problem!(m, c, A, b, cones, vartypes)
     if warmstart
-        set_warmstart!(m, problem, length(c), var_to_ranges)
+        set_warmstart!(m, problem, length(c), var_to_ranges, id_to_variables)
     end
     # optimize MPB conic problem
     MathProgBase.optimize!(m)
 
     # populate the status, the primal (and possibly dual) solution
     # and the primal (and possibly dual) variables with values
-    populate_solution!(m, problem, var_to_ranges, conic_constraints)
+    populate_solution!(m, problem, var_to_ranges, conic_constraints, id_to_variables, conic_constr_to_constr)
     if problem.status != :Optimal && verbose
         @warn "Problem status $(problem.status); solution may be inaccurate."
     end
@@ -55,7 +55,8 @@ end
 function set_warmstart!(m::MathProgBase.AbstractConicModel,
                         problem::Problem,
                         n::Int, # length of primal (conic) solution
-                        var_to_ranges)
+                        var_to_ranges,
+                        id_to_variables)
      # use previously cached solution, if any,
      try
          primal = problem.solution.primal
@@ -73,7 +74,7 @@ function set_warmstart!(m::MathProgBase.AbstractConicModel,
      end
 
      # grab any variables whose values the user may be trying to set
-     load_primal_solution!(primal, var_to_ranges)
+     load_primal_solution!(primal, var_to_ranges, id_to_variables)
 
      # notify the model that we're trying to warmstart
      try
@@ -105,7 +106,9 @@ end
 function populate_solution!(m::MathProgBase.AbstractConicModel,
                             problem::Problem,
                             var_to_ranges,
-                            conic_constraints)
+                            conic_constraints,
+                            id_to_variables,
+                            conic_constr_to_constr)
     dual = try
         MathProgBase.getdual(m)
     catch
@@ -130,10 +133,10 @@ function populate_solution!(m::MathProgBase.AbstractConicModel,
         problem.solution = Solution(solution, dual, MathProgBase.status(m), objective)
     end
 
-    populate_variables!(problem, var_to_ranges)
+    populate_variables!(problem, var_to_ranges, id_to_variables)
 
     if problem.solution.has_dual
-        populate_duals!(conic_constraints, problem.solution.dual)
+        populate_duals!(conic_constraints, problem.solution.dual, conic_constr_to_constr)
     end
 
     # minimize -> maximize
@@ -148,7 +151,7 @@ function populate_solution!(m::MathProgBase.AbstractConicModel,
     problem
 end
 
-function populate_variables!(problem::Problem, var_to_ranges::Dict{UInt64, Tuple{Int, Int}})
+function populate_variables!(problem::Problem, var_to_ranges::Dict{UInt64, Tuple{Int, Int}}, id_to_variables)
     x = problem.solution.primal
     for (id, (start_index, end_index)) in var_to_ranges
         var = id_to_variables[id]
@@ -174,7 +177,7 @@ end
 # TODO: it would be super cool to grab the other expressions that appear in the primal solution vector,
 # get their `expression_to_range`,
 # and populate them too using `evaluate`
-function load_primal_solution!(primal::Array{Float64,1}, var_to_ranges::Dict{UInt64, Tuple{Int, Int}})
+function load_primal_solution!(primal::Array{Float64,1}, var_to_ranges::Dict{UInt64, Tuple{Int, Int}}, id_to_variables)
     for (id, (start_index, end_index)) in var_to_ranges
         var = id_to_variables[id]
         if var.value !== nothing
@@ -188,7 +191,7 @@ function load_primal_solution!(primal::Array{Float64,1}, var_to_ranges::Dict{UIn
     end
 end
 
-function populate_duals!(constraints::Array{ConicConstr}, dual::Vector)
+function populate_duals!(constraints::Array{ConicConstr}, dual::Vector, conic_constr_to_constr)
     constr_index = 1
     for constraint in constraints
         # conic_constr_to_constr only has keys for conic constraints with a single objective

src/variable.jl

@@ -19,7 +19,6 @@ mutable struct Variable <: AbstractExpr
     function Variable(size::Tuple{Int, Int}, sign::Sign=NoSign(), sets::Symbol...)
         this = new(:variable, 0, nothing, size, AffineVexity(), sign, Symbol[sets...])
         this.id_hash = objectid(this)
-        id_to_variables[this.id_hash] = this
         return this
     end
 
@@ -53,11 +52,6 @@ function HermitianSemidefinite(m::Integer, n::Integer)
     end
 end
 
-# global map from unique variable ids to variables.
-# the expression tree will only utilize variable ids during construction
-# full information of the variables will be needed during stuffing
-# and after solving to populate the variables with values
-const id_to_variables = Dict{UInt64, Variable}()
 
 function vexity(x::Variable)
     return x.vexity
@@ -86,32 +80,6 @@ function imag_conic_form(x::Variable)
     end
 end
 
-function conic_form!(x::Variable, unique_conic_forms::UniqueConicForms=UniqueConicForms())
-    if !has_conic_form(unique_conic_forms, x)
-        if vexity(x) == ConstVexity()
-            # do exactly what we would for a constant
-            objective = ConicObj()
-            objective[objectid(:constant)] = (vec([real(x.value);]),vec([imag(x.value);]))
-            cache_conic_form!(unique_conic_forms, x, objective)
-        else
-            objective = ConicObj()
-            vec_size = length(x)
-
-            objective[x.id_hash] = (real_conic_form(x), imag_conic_form(x))
-            objective[objectid(:constant)] = (spzeros(vec_size, 1), spzeros(vec_size, 1))
-            # placeholder values in unique constraints prevent infinite recursion depth
-            cache_conic_form!(unique_conic_forms, x, objective)
-            if !(x.sign == NoSign() || x.sign == ComplexSign())
-                conic_form!(x.sign, x, unique_conic_forms)
-            end
-            for set in x.sets
-                conic_form!(set, x, unique_conic_forms)
-            end
-        end
-    end
-    return get_conic_form(unique_conic_forms, x)
-end
-
 # fix variables to hold them at their current value, and free them afterwards
 function fix!(x::Variable)
     x.value === nothing && error("This variable has no value yet; cannot fix value to nothing!")

src/variable_conic_form.jl

@@ -0,0 +1,26 @@
+function conic_form!(x::Variable, unique_conic_forms::UniqueConicForms=UniqueConicForms())
+    if !has_conic_form(unique_conic_forms, x)
+        add_to_id_to_variables!(unique_conic_forms, x)
+        if vexity(x) == ConstVexity()
+            # do exactly what we would for a constant
+            objective = ConicObj()
+            objective[objectid(:constant)] = (vec([real(x.value);]),vec([imag(x.value);]))
+            cache_conic_form!(unique_conic_forms, x, objective)
+        else
+            objective = ConicObj()
+            vec_size = length(x)
+
+            objective[x.id_hash] = (real_conic_form(x), imag_conic_form(x))
+            objective[objectid(:constant)] = (spzeros(vec_size, 1), spzeros(vec_size, 1))
+            # placeholder values in unique constraints prevent infinite recursion depth
+            cache_conic_form!(unique_conic_forms, x, objective)
+            if !(x.sign == NoSign() || x.sign == ComplexSign())
+                conic_form!(x.sign, x, unique_conic_forms)
+            end
+            for set in x.sets
+                conic_form!(set, x, unique_conic_forms)
+            end
+        end
+    end
+    return get_conic_form(unique_conic_forms, x)
+end

test/test_utilities.jl

@@ -55,20 +55,7 @@
     end
 
     @testset "clearmemory" begin
-        # solve a problem to populate globals
-        x = Variable()
-        p = minimize(-x, [x <= 0])
-        @test vexity(p) == AffineVexity()
-        solve!(p, solvers[1])
-
-        @test !isempty(Convex.id_to_variables)
-        @test !isempty(Convex.conic_constr_to_constr)
-
-        Convex.clearmemory()
-
-        # check they are cleared
-        @test isempty(Convex.id_to_variables)
-        @test isempty(Convex.conic_constr_to_constr)
+        @test_deprecated Convex.clearmemory()
     end
 
     @testset "ConicObj" for T = [UInt32, UInt64]