diff --git a/docs/src/examples/mixed_integer/aux_files/antidiag.jl b/docs/src/examples/mixed_integer/aux_files/antidiag.jl index dc488f85d..ec85595cc 100644 --- a/docs/src/examples/mixed_integer/aux_files/antidiag.jl +++ b/docs/src/examples/mixed_integer/aux_files/antidiag.jl @@ -14,9 +14,8 @@ export antidiag ### Diagonal ### Represents the kth diagonal of an mxn matrix as a (min(m, n) - k) x 1 vector -struct AntidiagAtom <: Convex.AbstractExpr +mutable struct AntidiagAtom <: Convex.AbstractExpr head::Symbol - id_hash::UInt64 children::Tuple{Convex.AbstractExpr} size::Tuple{Int,Int} k::Int @@ -27,13 +26,7 @@ struct AntidiagAtom <: Convex.AbstractExpr error("Bounds error in calling diag") end children = (x,) - return new( - :antidiag, - hash((children, k)), - children, - (minimum(x.size) - k, 1), - k, - ) + return new(:antidiag, children, (minimum(x.size) - k, 1), k) end end diff --git a/docs/src/manual/advanced.md b/docs/src/manual/advanced.md index a98a910af..37aecdf51 100644 --- a/docs/src/manual/advanced.md +++ b/docs/src/manual/advanced.md @@ -131,16 +131,15 @@ this. To do so, we define ```@example 1 using Convex + +# Must be mutable! Otherwise variables with the same size/value would be treated as the same object. mutable struct ProbabilityVector <: Convex.AbstractVariable head::Symbol - id_hash::UInt64 - size::Tuple{Int, Int} + size::Tuple{Int,Int} value::Union{Convex.Value,Nothing} vexity::Convex.Vexity function ProbabilityVector(d) - this = new(:ProbabilityVector, 0, (d,1), nothing, Convex.AffineVexity()) - this.id_hash = objectid(this) - this + return new(:ProbabilityVector, (d, 1), nothing, Convex.AffineVexity()) end end @@ -165,8 +164,8 @@ solve!(prob, SCS.Optimizer) evaluate(p) # [1.0, 0.0, 0.0] ``` -Subtypes of `AbstractVariable` must have the fields `head`, `id_hash`, and -`size`, and `id_hash` must be populated as shown in the example. Then they must also +Subtypes of `AbstractVariable` must have the fields `head` and +`size`. Then they must also * either have a field `value`, or implement [`Convex._value`](@ref) and [`Convex.set_value!`](@ref) diff --git a/src/Context.jl b/src/Context.jl index 3ab5d4e48..d3dbc908e 100644 --- a/src/Context.jl +++ b/src/Context.jl @@ -8,23 +8,20 @@ mutable struct Context{T,M} model::M # Used for populating variable values after solving - var_id_to_moi_indices::OrderedCollections.OrderedDict{ - UInt64, + var_to_moi_indices::IdDict{ + Any, Union{ Vector{MOI.VariableIndex}, Tuple{Vector{MOI.VariableIndex},Vector{MOI.VariableIndex}}, }, } - # `id_hash` -> `AbstractVariable` - id_to_variables::OrderedCollections.OrderedDict{UInt64,Any} # Used for populating constraint duals constr_to_moi_inds::IdDict{Any,Any} - detected_infeasible_during_formulation::Ref{Bool} + detected_infeasible_during_formulation::Bool # Cache - # conic_form_cache::DataStructures.WeakKeyIdDict{Any, Any} conic_form_cache::IdDict{Any,Any} end @@ -39,8 +36,13 @@ function Context{T}(optimizer_factory; add_cache::Bool = false) where {T} end return Context{T,typeof(model)}( model, - OrderedCollections.OrderedDict{UInt64,Vector{MOI.VariableIndex}}(), - OrderedCollections.OrderedDict{UInt64,Any}(), + IdDict{ + Any, + Union{ + Vector{MOI.VariableIndex}, + Tuple{Vector{MOI.VariableIndex},Vector{MOI.VariableIndex}}, + }, + }(), IdDict{Any,Any}(), false, IdDict{Any,Any}(), @@ -49,10 +51,9 @@ end function Base.empty!(context::Context) MOI.empty!(context.model) - empty!(context.var_id_to_moi_indices) - empty!(context.id_to_variables) + empty!(context.var_to_moi_indices) empty!(context.constr_to_moi_inds) - context.detected_infeasible_during_formulation[] = false + context.detected_infeasible_during_formulation = false empty!(context.conic_form_cache) return end diff --git a/src/MOI_wrapper.jl b/src/MOI_wrapper.jl index 7fb73c212..433491c61 100644 --- a/src/MOI_wrapper.jl +++ b/src/MOI_wrapper.jl @@ -5,7 +5,7 @@ struct Optimizer{T,M} <: MOI.AbstractOptimizer context::Context{T,M} - moi_to_convex::OrderedCollections.OrderedDict{MOI.VariableIndex,UInt64} + moi_to_convex::OrderedCollections.OrderedDict{MOI.VariableIndex,Any} convex_to_moi::Dict{UInt64,Vector{MOI.VariableIndex}} constraint_map::Vector{MOI.ConstraintIndex} function Optimizer(context::Context{T,M}) where {T,M} @@ -47,9 +47,8 @@ end function _add_variable(model::Optimizer, vi::MOI.VariableIndex) var = Variable() - model.moi_to_convex[vi] = var.id_hash - model.context.var_id_to_moi_indices[var.id_hash] = [vi] - model.context.id_to_variables[var.id_hash] = var + model.moi_to_convex[vi] = var + model.context.var_to_moi_indices[var] = [vi] return end @@ -129,7 +128,7 @@ function _expr(::Optimizer, v::Value) end function _expr(model::Optimizer, x::MOI.VariableIndex) - return model.context.id_to_variables[model.moi_to_convex[x]] + return model.moi_to_convex[x] end function _expr(model::Optimizer, f::MOI.AbstractScalarFunction) diff --git a/src/constant.jl b/src/constant.jl index 81c71d69a..21e812925 100644 --- a/src/constant.jl +++ b/src/constant.jl @@ -38,7 +38,6 @@ end mutable struct Constant{T<:Real} <: AbstractExpr head::Symbol - id_hash::UInt64 value::Union{Matrix{T},SPARSE_MATRIX{T}} size::Tuple{Int,Int} sign::Sign @@ -47,13 +46,7 @@ mutable struct Constant{T<:Real} <: AbstractExpr if x isa Complex || x isa AbstractArray{<:Complex} throw(DomainError(x, "Constant expects real values")) end - return new{eltype(x)}( - :constant, - objectid(x), - _matrix(x), - _size(x), - sign, - ) + return new{eltype(x)}(:constant, _matrix(x), _size(x), sign) end end function Constant(x::Value, check_sign::Bool = true) @@ -63,13 +56,12 @@ end mutable struct ComplexConstant{T<:Real} <: AbstractExpr head::Symbol - id_hash::UInt64 size::Tuple{Int,Int} real_constant::Constant{T} imag_constant::Constant{T} function ComplexConstant(re::Constant{T}, im::Constant{T}) where {T} size(re) == size(im) || error("size mismatch") - return new{T}(:complex_constant, rand(UInt64), size(re), re, im) + return new{T}(:complex_constant, size(re), re, im) end # function ComplexConstant(re::Constant{S1}, im::Constant{S2}) where {S1,S2} diff --git a/src/constraints/GenericConstraint.jl b/src/constraints/GenericConstraint.jl index d4cd2cffd..0063da0e1 100644 --- a/src/constraints/GenericConstraint.jl +++ b/src/constraints/GenericConstraint.jl @@ -87,7 +87,7 @@ end function _add_constraint!(context::Context, c::GenericConstraint) if vexity(c.child) == ConstVexity() if !is_feasible(evaluate(c.child), c.set, CONSTANT_CONSTRAINT_TOL[]) - context.detected_infeasible_during_formulation[] = true + context.detected_infeasible_during_formulation = true end return end diff --git a/src/expressions.jl b/src/expressions.jl index 8ca8e5534..3cce9631f 100644 --- a/src/expressions.jl +++ b/src/expressions.jl @@ -40,8 +40,7 @@ const Value = Union{Number,AbstractArray} # We commandeer `==` to create a constraint. # Therefore we define `isequal` to still have a notion of equality # (Normally `isequal` falls back to `==`, so we need to provide a method). -# All `AbstractExpr` (Constraints are not AbstractExpr's!) are compared by value, except for AbstractVariables, -# which use their `id_hash` field. +# All `AbstractExpr` (Constraints are not AbstractExpr's!) are compared by value, except for AbstractVariables, which are compared by `===` (objectid). function Base.isequal(x::AbstractExpr, y::AbstractExpr) if typeof(x) != typeof(y) return false diff --git a/src/solution.jl b/src/solution.jl index 25037d495..282600505 100644 --- a/src/solution.jl +++ b/src/solution.jl @@ -39,8 +39,7 @@ end function _add_variable_primal_start(context::Convex.Context{T}) where {T} attr = MOI.VariablePrimalStart() - for (id, moi_indices) in context.var_id_to_moi_indices - x = context.id_to_variables[id] + for (x, moi_indices) in context.var_to_moi_indices if x.value === nothing continue elseif moi_indices isa Tuple # Variable is complex @@ -97,7 +96,7 @@ function solve!( if warmstart && MOI.supports(context.model, attr, MOI.VariableIndex) _add_variable_primal_start(context) end - if context.detected_infeasible_during_formulation[] + if context.detected_infeasible_during_formulation p.status = MOI.INFEASIBLE else MOI.optimize!(context.model) @@ -108,8 +107,7 @@ function solve!( @warn "Problem wasn't solved optimally" status = p.status end primal_status = MOI.get(context.model, MOI.PrimalStatus()) - for (id, indices) in context.var_id_to_moi_indices - var = context.id_to_variables[id] + for (var, indices) in context.var_to_moi_indices if vexity(var) == ConstVexity() continue # Fixed variable elseif primal_status == MOI.NO_SOLUTION diff --git a/src/utilities/show.jl b/src/utilities/show.jl index 10f6cbffe..35973db81 100644 --- a/src/utilities/show.jl +++ b/src/utilities/show.jl @@ -8,7 +8,7 @@ using .TreePrint """ show_id(io::IO, x::Union{AbstractVariable}; digits = 3) -Print a truncated version of the objects `id_hash` field. +Print a truncated version of the object's id. ## Example @@ -19,12 +19,12 @@ julia> Convex.show_id(stdout, x) id: 163…906 ``` """ -function show_id(io::IO, x::Union{AbstractVariable}; digits = MAXDIGITS[]) +function show_id(io::IO, x::AbstractVariable; digits = MAXDIGITS[]) return print(io, show_id(x; digits = digits)) end -function show_id(x::Union{AbstractVariable}; digits = MAXDIGITS[]) - hash_str = string(x.id_hash) +function show_id(x::AbstractVariable; digits = MAXDIGITS[]) + hash_str = string(objectid(x)) if length(hash_str) > (2 * digits + 1) return "id: " * first(hash_str, digits) * "…" * last(hash_str, digits) else diff --git a/src/variable.jl b/src/variable.jl index dab53bd76..13693c7e7 100644 --- a/src/variable.jl +++ b/src/variable.jl @@ -18,8 +18,7 @@ integer-valued (`IntVar`), or binary (`BinVar`). """ abstract type AbstractVariable <: AbstractExpr end -An `AbstractVariable` should have `head` field, an `id_hash` field -and a `size` field to conform to the `AbstractExpr` interface, and +An `AbstractVariable` should have `head` field, and a `size` field to conform to the `AbstractExpr` interface, and implement methods (or use the field-access fallbacks) for * [`_value`](@ref), [`set_value!`](@ref): get or set the numeric value of the variable. @@ -188,10 +187,10 @@ function free!(x::AbstractVariable) end function Base.isequal(x::AbstractVariable, y::AbstractVariable) - return x.id_hash == y.id_hash + return x === y end -Base.hash(x::AbstractVariable, h::UInt) = hash(x.id_hash, h) +Base.hash(x::AbstractVariable, h::UInt) = hash(objectid(x), h) iscomplex(x::Sign) = x == ComplexSign() @@ -204,8 +203,6 @@ iscomplex(::Union{Real,AbstractArray{<:Real}}) = false mutable struct Variable <: AbstractVariable # Every `AbstractExpr` has a `head`; for a Variable it is set to `:variable`. head::Symbol - # A unique identifying hash used for caching. - id_hash::UInt64 # The current value of the variable. Defaults to `nothing` until the # variable has been [`fix!`](@ref)'d to a particular value, or the # variable has been used in a problem which has been solved, at which @@ -243,9 +240,8 @@ mutable struct Variable <: AbstractVariable ), ) end - this = new( + return new( :variable, - 0, nothing, size, AffineVexity(), @@ -253,8 +249,6 @@ mutable struct Variable <: AbstractVariable Constraint[], vartype, ) - this.id_hash = objectid(this) - return this end end @@ -274,7 +268,6 @@ Variable(vartype::VarType) = Variable((1, 1), NoSign(), vartype) mutable struct ComplexVariable <: AbstractVariable head::Symbol - id_hash::UInt64 size::Tuple{Int,Int} value::Union{Value,Nothing} vexity::Vexity @@ -283,7 +276,6 @@ mutable struct ComplexVariable <: AbstractVariable function ComplexVariable(size::Tuple{Int,Int} = (1, 1)) return new( :ComplexVariable, - rand(UInt64), size, nothing, AffineVexity(), diff --git a/src/variable_template.jl b/src/variable_template.jl index 88200e3ad..0da01846d 100644 --- a/src/variable_template.jl +++ b/src/variable_template.jl @@ -6,15 +6,13 @@ # It might be useful to get a direct VOV sometimes... function _template(a::AbstractVariable, context::Context{T}) where {T} first_cache = false - var_inds = get!(context.var_id_to_moi_indices, a.id_hash) do + var_inds = get!(context.var_to_moi_indices, a) do first_cache = true return add_variables!(context.model, a) end - context.id_to_variables[a.id_hash] = a - # we only want this to run once, when the variable is first added, - # and after `var_id_to_moi_indices` is populated + # and after `var_to_moi_indices` is populated if first_cache if sign(a) == Positive() add_constraint!(context, a >= 0) @@ -97,18 +95,6 @@ accessed in `context[a]`, otherwise, it is created by calling with the same expression does not create a duplicate one. """ function conic_form!(context::Context, a::AbstractExpr) - - # Nicer implementation d = context.conic_form_cache return get!(() -> new_conic_form!(context, a), d, a) - - # Avoid closure - # wkh = context.conic_form_cache - # default = () -> new_conic_form!(context, a) - # key = a - # return Base.@lock wkh.lock begin - # get!(default, wkh.ht, DataStructures.WeakRefForWeakDict(key)) - # end - - return end diff --git a/test/test_utilities.jl b/test/test_utilities.jl index 381cb23d8..3c81d78cd 100644 --- a/test/test_utilities.jl +++ b/test/test_utilities.jl @@ -279,7 +279,7 @@ function test_show() old_maxdigits = Convex.MAXDIGITS[] Convex.MAXDIGITS[] = 100 @test length(Convex.show_id(x)) == - length("id: ") + length(string(x.id_hash)) + length("id: ") + length(string(objectid(x))) Convex.MAXDIGITS[] = old_maxdigits return end @@ -764,16 +764,15 @@ using Convex # To make sure `Convex` isn't using field access on `AbstractVariable`'s # we'll use a global dictionary to store information about each instance # our of mock variable type, `DictVector`. -const global_cache = Dict{UInt64,Any}() +const global_cache = IdDict{Any,Any}() +# Must be mutable to have unique object identity even if identical fields mutable struct DictVector{T} <: Convex.AbstractVariable head::Symbol - id_hash::UInt64 size::Tuple{Int,Int} function DictVector{T}(d) where {T} - this = new(:DictVector, 0, (d, 1)) - this.id_hash = objectid(this) - global_cache[this.id_hash] = Dict( + this = new(:DictVector, (d, 1)) + global_cache[this] = Dict( :value => nothing, :sign => T <: Complex ? ComplexSign() : NoSign(), :vartype => ContVar, @@ -784,36 +783,36 @@ mutable struct DictVector{T} <: Convex.AbstractVariable end end -Convex.evaluate(x::DictVector) = global_cache[x.id_hash][:value] +Convex.evaluate(x::DictVector) = global_cache[x][:value] function Convex.set_value!(x::DictVector, v::AbstractArray) - return global_cache[x.id_hash][:value] = v + return global_cache[x][:value] = v end function Convex.set_value!(x::DictVector, v::Number) - return global_cache[x.id_hash][:value] = v + return global_cache[x][:value] = v end -Convex.vexity(x::DictVector) = global_cache[x.id_hash][:vexity] +Convex.vexity(x::DictVector) = global_cache[x][:vexity] function Convex.vexity!(x::DictVector, v::Convex.Vexity) - return global_cache[x.id_hash][:vexity] = v + return global_cache[x][:vexity] = v end -Convex.sign(x::DictVector) = global_cache[x.id_hash][:sign] +Convex.sign(x::DictVector) = global_cache[x][:sign] -Convex.sign!(x::DictVector, s::Convex.Sign) = global_cache[x.id_hash][:sign] = s +Convex.sign!(x::DictVector, s::Convex.Sign) = global_cache[x][:sign] = s -Convex.vartype(x::DictVector) = global_cache[x.id_hash][:vartype] +Convex.vartype(x::DictVector) = global_cache[x][:vartype] function Convex.vartype!(x::DictVector, s::Convex.VarType) - return global_cache[x.id_hash][:vartype] = s + return global_cache[x][:vartype] = s end -Convex.get_constraints(x::DictVector) = global_cache[x.id_hash][:constraints] +Convex.get_constraints(x::DictVector) = global_cache[x][:constraints] function Convex.add_constraint!(x::DictVector, s::Convex.Constraint) - return push!(global_cache[x.id_hash][:constraints], s) + return push!(global_cache[x][:constraints], s) end end # DictVector @@ -842,20 +841,16 @@ using Convex mutable struct DensityMatrix{T} <: Convex.AbstractVariable head::Symbol - id_hash::UInt64 size::Tuple{Int,Int} value::Union{Convex.Value,Nothing} vexity::Convex.Vexity function DensityMatrix(d) - this = new{ComplexF64}( + return new{ComplexF64}( :DensityMatrix, - 0, (d, d), nothing, Convex.AffineVexity(), ) - this.id_hash = objectid(this) - return this end end @@ -898,15 +893,12 @@ using Convex mutable struct ProbabilityVector <: Convex.AbstractVariable head::Symbol - id_hash::UInt64 size::Tuple{Int,Int} value::Union{Convex.Value,Nothing} vexity::Convex.Vexity + function ProbabilityVector(d) - this = - new(:ProbabilityVector, 0, (d, 1), nothing, Convex.AffineVexity()) - this.id_hash = objectid(this) - return this + return new(:ProbabilityVector, (d, 1), nothing, Convex.AffineVexity()) end end @@ -1285,7 +1277,7 @@ function test_variable_primal_start() x4 => ([1.0], [2.0]), x5 => ([1.0, 0.0, 2.0, 4.0], [0.0, 3.0, 0.0, 0.0]), ) - variable = context.var_id_to_moi_indices[key.id_hash] + variable = context.var_to_moi_indices[key] start = if variable isa Tuple ( MOI.get(context.model, MOI.VariablePrimalStart(), variable[1]),