[breaking] Cleanup the Context object: remove id_hash, simplify dictionaries (#645)

* `detected_infeasible_during_formulation` does not need to be a ref

* rm `id_hash`es, use `IdDict` directly

* format

Author: ericphanson

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
 

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)

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

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)

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}

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

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

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

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

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,18 +240,15 @@ mutable struct Variable <: AbstractVariable
                 ),
             )
         end
-        this = new(
+        return new(
             :variable,
-            0,
             nothing,
             size,
             AffineVexity(),
             sign,
             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(),