Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove global variables #322

Merged
merged 5 commits into from
Sep 10, 2019
Merged

Remove global variables #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b0fc2d
Remove global `id_to_variables`
ericphanson Aug 31, 2019
487c5c9
Use better name for method
ericphanson Aug 31, 2019
09e278c
Remove the other global, `conic_constr_to_constr`
ericphanson Sep 1, 2019
86f5067
Remove `GC.gc()` from `Convex.clearmemory()`
ericphanson Sep 1, 2019
26f73e9
Remove commented line
ericphanson Sep 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/Convex.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -83,13 +86,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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that GC.gc() should never be mandatory in user code as Convex, the function could be removed altogether

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's true. I thought it should at least be deprecated though, to give users time to remove it so they aren't hit with errors upon upgrading Convex if they have Convex.clearmemory() in their scripts. I wasn't sure about leaving in the GC.gc() or not; I agree they shouldn't need to be using it, but it was something that Convex.clearmemory() was doing, so I thought maybe I should point to it as a replacement in case somehow that's a key part of whatever they are doing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the wording and removed the GC.gc() bit. I don't see any harm in leaving a deprecation message in for a month or two though, to let users comfortably remove the function without getting errors.

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
2 changes: 1 addition & 1 deletion src/atoms/affine/stack.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 10 additions & 2 deletions src/conic_form.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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
10 changes: 4 additions & 6 deletions src/constraints/constraints.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
9 changes: 6 additions & 3 deletions src/problems.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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...) =
Expand Down
25 changes: 14 additions & 11 deletions src/solution.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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]
Expand All @@ -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
Expand All @@ -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
Expand Down
32 changes: 0 additions & 32 deletions src/variable.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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!")
Expand Down
26 changes: 26 additions & 0 deletions src/variable_conic_form.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
15 changes: 1 addition & 14 deletions test/test_utilities.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,7 @@
@testset "Utilities" begin

@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]
Expand Down