[Utilities] improve errors and docstrings for querying attributes (#2350

)
jump-dev · Nov 11, 2023 · 0f8d35c · 0f8d35c
1 parent fd619d2
commit 0f8d35c
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 100 deletions.
diff --git a/src/Utilities/cachingoptimizer.jl b/src/Utilities/cachingoptimizer.jl
@@ -864,15 +864,34 @@ function _get_model_attribute(
     )
 end
 
+function _throw_if_get_attribute_not_allowed(
+    model::CachingOptimizer,
+    attr;
+    needs_optimizer_map::Bool,
+)
+    # If the state(model) == EMPTY_OPTIMIZER, then
+    # `model.model_to_optimizer_map[index]` might be empty (because copy_to
+    # has not been called yet), or it might be full, if optimize!(dest, src)
+    # did not leave a copy in `dest`.
+    missing_map = needs_optimizer_map && isempty(model.model_to_optimizer_map)
+    if state(model) == NO_OPTIMIZER || missing_map
+        msg =
+            "Cannot query $(attr) from `Utilities.CachingOptimizer` " *
+            "because no optimizer is attached (the state is `$(state(model))`)."
+        throw(MOI.GetAttributeNotAllowed(attr, msg))
+    end
+    return
+end
+
 function MOI.get(model::CachingOptimizer, attr::MOI.AbstractModelAttribute)
     if !MOI.is_set_by_optimize(attr)
         return MOI.get(model.model_cache, attr)
-    elseif state(model) == NO_OPTIMIZER
-        error(
-            "Cannot query $(attr) from caching optimizer because no " *
-            "optimizer is attached.",
-        )
     end
+    _throw_if_get_attribute_not_allowed(
+        model,
+        attr;
+        needs_optimizer_map = false,
+    )
     return _get_model_attribute(model, attr)
 end
 
@@ -901,51 +920,33 @@ function MOI.get(
     attr::Union{MOI.AbstractVariableAttribute,MOI.AbstractConstraintAttribute},
     index::MOI.Index,
 )
-    if MOI.is_set_by_optimize(attr)
-        if state(model) == NO_OPTIMIZER
-            error(
-                "Cannot query $(attr) from caching optimizer because no " *
-                "optimizer is attached.",
-            )
-        end
-        return map_indices(
-            model.optimizer_to_model_map,
-            attr,
-            MOI.get(
-                model.optimizer,
-                attr,
-                model.model_to_optimizer_map[index],
-            )::MOI.attribute_value_type(attr),
-        )
-    else
+    if !MOI.is_set_by_optimize(attr)
         return MOI.get(model.model_cache, attr, index)
     end
+    _throw_if_get_attribute_not_allowed(model, attr; needs_optimizer_map = true)
+    value = MOI.get(
+        model.optimizer,
+        attr,
+        model.model_to_optimizer_map[index],
+    )::MOI.attribute_value_type(attr)
+    return map_indices(model.optimizer_to_model_map, attr, value)
 end
 
 function MOI.get(
     model::CachingOptimizer,
     attr::Union{MOI.AbstractVariableAttribute,MOI.AbstractConstraintAttribute},
     indices::Vector{<:MOI.Index},
 )
-    if MOI.is_set_by_optimize(attr)
-        if state(model) == NO_OPTIMIZER
-            error(
-                "Cannot query $(attr) from caching optimizer because no " *
-                "optimizer is attached.",
-            )
-        end
-        return map_indices(
-            model.optimizer_to_model_map,
-            attr,
-            MOI.get(
-                model.optimizer,
-                attr,
-                map(index -> model.model_to_optimizer_map[index], indices),
-            )::Vector{<:MOI.attribute_value_type(attr)},
-        )
-    else
+    if !MOI.is_set_by_optimize(attr)
         return MOI.get(model.model_cache, attr, indices)
     end
+    _throw_if_get_attribute_not_allowed(model, attr; needs_optimizer_map = true)
+    value = MOI.get(
+        model.optimizer,
+        attr,
+        map(Base.Fix1(getindex, model.model_to_optimizer_map), indices),
+    )::Vector{<:MOI.attribute_value_type(attr)}
+    return map_indices(model.optimizer_to_model_map, attr, value)
 end
 
 ###
@@ -961,12 +962,7 @@ function MOI.get(
     attr::MOI.ConstraintPrimal,
     index::MOI.ConstraintIndex,
 )
-    if state(model) == NO_OPTIMIZER
-        error(
-            "Cannot query $(attr) from caching optimizer because no " *
-            "optimizer is attached.",
-        )
-    end
+    _throw_if_get_attribute_not_allowed(model, attr; needs_optimizer_map = true)
     try
         return MOI.get(
             model.optimizer,
@@ -987,12 +983,7 @@ function MOI.get(
     attr::MOI.ConstraintPrimal,
     indices::Vector{<:MOI.ConstraintIndex},
 )
-    if state(model) == NO_OPTIMIZER
-        error(
-            "Cannot query $(attr) from caching optimizer because no " *
-            "optimizer is attached.",
-        )
-    end
+    _throw_if_get_attribute_not_allowed(model, attr; needs_optimizer_map = true)
     try
         return MOI.get(
             model.optimizer,
@@ -1055,16 +1046,15 @@ function MOI.set(
 end
 
 function MOI.get(model::CachingOptimizer, attr::MOI.AbstractOptimizerAttribute)
-    if state(model) == NO_OPTIMIZER
-        # TODO: Copyable attributes (e.g., `Silent`, `TimeLimitSec`,
-        # `NumberOfThreads`) should also be stored in the cache so we could
-        # return the value stored in the cache instead. However, for
-        # non-copyable attributes( e.g. `SolverName`) the error is appropriate.
-        error(
-            "Cannot query $(attr) from caching optimizer because no " *
-            "optimizer is attached.",
-        )
-    end
+    # TODO: Copyable attributes (e.g., `Silent`, `TimeLimitSec`,
+    # `NumberOfThreads`) should also be stored in the cache so we could
+    # return the value stored in the cache instead. However, for
+    # non-copyable attributes( e.g. `SolverName`) the error is appropriate.
+    _throw_if_get_attribute_not_allowed(
+        model,
+        attr;
+        needs_optimizer_map = false,
+    )
     return map_indices(
         model.optimizer_to_model_map,
         attr,

diff --git a/src/attributes.jl b/src/attributes.jl
@@ -2242,14 +2242,23 @@ end
 """
     is_set_by_optimize(::AnyAttribute)
 
-Return a `Bool` indicating whether the value of the attribute is modified
-during an [`optimize!`](@ref) call, that is, the attribute is used to query
-the result of the optimization.
+Return a `Bool` indicating whether the value of the attribute is set during an
+[`optimize!`](@ref) call, that is, the attribute is used to query the result of
+the optimization.
 
-## Important note when defining new attributes
+If an attibute can be set by the user, define [`is_copyable`](@ref) instead.
+
+An attribute cannot be both [`is_copyable`](@ref) and `is_set_by_optimize`.
+
+## Default fallback
 
 This function returns `false` by default so it should be implemented for
-attributes that are modified by [`optimize!`](@ref).
+attributes that are set by [`optimize!`](@ref).
+
+## Undefined behavior
+
+Querying the value of the attribute that `is_set_by_optimize` before a call to
+[`optimize!`](@ref) is undefined and depends on solver-specific behavior.
 """
 is_set_by_optimize(::AnyAttribute) = false
 
@@ -2287,27 +2296,20 @@ end
 Return a `Bool` indicating whether the value of the attribute may be copied
 during [`copy_to`](@ref) using [`set`](@ref).
 
-## Important note when defining new attributes
-
-By default `is_copyable(attr)` returns `!is_set_by_optimize(attr)`. A specific
-method should be defined for attributes which are copied indirectly during
-[`copy_to`](@ref). For instance, both `is_copyable` and
-[`is_set_by_optimize`](@ref) return `false` for the following attributes:
-
-* [`ListOfOptimizerAttributesSet`](@ref), [`ListOfModelAttributesSet`](@ref),
-  [`ListOfConstraintAttributesSet`](@ref) and
-  [`ListOfVariableAttributesSet`](@ref).
-* [`SolverName`](@ref) and [`RawSolver`](@ref): these attributes cannot be set.
-* [`NumberOfVariables`](@ref) and [`ListOfVariableIndices`](@ref): these
-  attributes are set indirectly by [`add_variable`](@ref) and
-  [`add_variables`](@ref).
-* [`ObjectiveFunctionType`](@ref): this attribute is set indirectly when setting
-  the [`ObjectiveFunction`](@ref) attribute.
-* [`NumberOfConstraints`](@ref), [`ListOfConstraintIndices`](@ref),
-  [`ListOfConstraintTypesPresent`](@ref), [`CanonicalConstraintFunction`](@ref),
-  [`ConstraintFunction`](@ref) and [`ConstraintSet`](@ref):
-  these attributes are set indirectly by
-  [`add_constraint`](@ref) and [`add_constraints`](@ref).
+If an attribute `is_copyable`, then it cannot be modified by the optimizer, and
+[`get`](@ref) must always return the value that was [`set`](@ref) by the user.
+
+If an attibute is the result of an optimization, define
+[`is_set_by_optimize`](@ref) instead.
+
+An attribute cannot be both [`is_set_by_optimize`](@ref) and `is_copyable`.
+
+## Default fallback
+
+By default `is_copyable(attr)` returns `!is_set_by_optimize(attr)`, which is
+most probably `true`.
+
+If an attribute should not be copied, define `is_copyable(::MyAttribute) = false`.
 """
 function is_copyable(attr::AnyAttribute)
     return !is_set_by_optimize(attr)

diff --git a/test/Utilities/cachingoptimizer.jl b/test/Utilities/cachingoptimizer.jl
@@ -179,13 +179,14 @@ function test_default_attributes()
     @test MOI.get(model, MOI.PrimalStatus()) == MOI.NO_SOLUTION
     @test MOI.get(model, MOI.DualStatus()) == MOI.NO_SOLUTION
     x = MOI.add_variables(model, 2)
-    attr = MOI.VariablePrimal()
-    exception = ErrorException(
-        "Cannot query $(attr) from caching optimizer because no optimizer" *
-        " is attached.",
+    @test_throws(
+        MOI.GetAttributeNotAllowed{MOI.VariablePrimal},
+        MOI.get(model, MOI.VariablePrimal(), x[1]),
+    )
+    @test_throws(
+        MOI.GetAttributeNotAllowed{MOI.VariablePrimal},
+        MOI.get(model, MOI.VariablePrimal(), x),
     )
-    @test_throws exception MOI.get(model, MOI.VariablePrimal(), x[1])
-    @test_throws exception MOI.get(model, MOI.VariablePrimal(), x)
     for attr in (
         MOI.SolverName(),
         MOI.Silent(),
@@ -195,10 +196,7 @@ function test_default_attributes()
         MOI.ResultCount(),
     )
         @test_throws(
-            ErrorException(
-                "Cannot query $(attr) from caching optimizer because no " *
-                "optimizer is attached.",
-            ),
+            MOI.GetAttributeNotAllowed{typeof(attr)},
             MOI.get(model, attr),
         )
     end
@@ -450,7 +448,7 @@ function test_CachingOptimizer_MANUAL_mode()
         typeof(v),
     )
     MOI.set(m, MOIU.AttributeFromOptimizer(MOI.VariablePrimal()), v, 3.0)
-
+    MOI.set(s, MOI.TerminationStatus(), MOI.OPTIMAL)
     MOI.optimize!(m)
 
     @test MOI.get(m, MOI.VariablePrimal(), v) == 3.0
@@ -828,6 +826,8 @@ MOI.get(::_GetFallbackModel1310, ::MOI.PrimalStatus) = MOI.FEASIBLE_POINT
 
 MOI.get(::_GetFallbackModel1310, ::MOI.DualStatus) = MOI.FEASIBLE_POINT
 
+MOI.get(::_GetFallbackModel1310, ::MOI.TerminationStatus) = MOI.OTHER_ERROR
+
 MOI.get(::_GetFallbackModel1310, ::MOI.ResultCount) = 1
 
 function MOI.optimize!(::_GetFallbackModel1310, model::MOI.ModelLike)
@@ -887,8 +887,14 @@ function test_ConstraintPrimal_fallback_error()
     )
     x = MOI.add_variable(model)
     c = MOI.add_constraint(model, x, MOI.GreaterThan(1.0))
-    @test_throws(ErrorException, MOI.get(model, MOI.ConstraintPrimal(), c))
-    @test_throws(ErrorException, MOI.get(model, MOI.ConstraintPrimal(), [c]))
+    @test_throws(
+        MOI.GetAttributeNotAllowed{MOI.ConstraintPrimal},
+        MOI.get(model, MOI.ConstraintPrimal(), c),
+    )
+    @test_throws(
+        MOI.GetAttributeNotAllowed{MOI.ConstraintPrimal},
+        MOI.get(model, MOI.ConstraintPrimal(), [c]),
+    )
     return
 end