[Utilities] simplify MutableSparseMatrix to remove final_touch

[odow]

Closes #1711

The need for final_touch is just a recipe for bugs like this. We should endeavor to make it that we don't need to call final_touch for most things (now I think it's only the sets).

[odow]

Let's get @blegat to take a look

[blegat]

I would prefer the same approach as with MatrixOfConstraints. A final_touch boolean and convert checks that it has been called.
It could even be more than a boolean. There is three states: allocating, loading and done.
And you have the transitions: allocating -set_number_of_rows-> loading -final_touch-> done -MOI.empty!-> allocating.
We could have an enum with the three states and always check that we are on the correct state.

[joaquimg]

If the operations must follow a precise order I agree with an enum and @asserts to guarantee the order.
I like the new implementation with a no-op final_touch, am I missing any performance bottleneck?

I imagine that we can add the enum as extra protection and keep the current no-op final_touch. To get the best of both worlds.

About checking, I agree its fine to check final_touch in convert as extra protection, but actually relying on that would be worse. Or we should at least document that the user should always call convert or an is_done thing.

[blegat]

The extra vector could also be used to do sanity checks at final_touch if we want to check that the same amont that was allocated was also loaded but that can be left for another PR.

[odow]

I dug into this more. The problem is our lazy creation of the constraint objects:

julia> model = MOI.Utilities.CachingOptimizer(
           MOI.Utilities.UniversalFallback(MOI.Utilities.Model{Float64}()),
           MOI.Utilities.AUTOMATIC,
       )
MOIU.CachingOptimizer{MOI.AbstractOptimizer, MOIU.UniversalFallback{MOIU.Model{Float64}}}
in state NO_OPTIMIZER
in mode AUTOMATIC
with model cache MOIU.UniversalFallback{MOIU.Model{Float64}}
  fallback for MOIU.Model{Float64}
with optimizer nothing

julia> x = MOI.add_variable(model)
MathOptInterface.VariableIndex(1)

julia> cache = OptimizerCache{Float64}()
MOIU.GenericModel{Float64, MOIU.ObjectiveContainer{Float64}, MOIU.VariablesContainer{Float64}, StructCache{Float64, MOIU.MatrixOfConstraints{Float64, MOIU.MutableSparseMatrixCSC{Float64, Int64, MOIU.OneBasedIndexing}, Vector{Float64}, Zeros{Float64}}, MOIU.MatrixOfConstraints{Float64, MOIU.MutableSparseMatrixCSC{Float64, Int64, MOIU.OneBasedIndexing}, Vector{Float64}, Nonpositives{Float64}}}}

julia> index_map = MOI.copy_to(cache, model)
MathOptInterface.Utilities.IndexMap with 1 entry:
  VariableIndex(1) => VariableIndex(1)

julia> cache.constraints.moi_nonpositives

At this point, the field is still nothing. It's only when we access the field that it gets created:

julia> Gh = MOI.Utilities.constraints(
           cache.constraints,
           MOI.VectorAffineFunction{Float64},
           MOI.Nonpositives,
       )
MOIU.MatrixOfConstraints{Float64, MOIU.MutableSparseMatrixCSC{Float64, Int64, MOIU.OneBasedIndexing}, Vector{Float64}, Nonpositives{Float64}}

julia> G = Gh.coefficients
MathOptInterface.Utilities.MutableSparseMatrixCSC{Float64, Int64, MathOptInterface.Utilities.OneBasedIndexing}(MathOptInterface.Utilities.OneBasedIndexing(), 0, 1, [1, 1], Int64[], Float64[], [0])

julia> cache.constraints.moi_nonpositives
MOIU.MatrixOfConstraints{Float64, MOIU.MutableSparseMatrixCSC{Float64, Int64, MOIU.OneBasedIndexing}, Vector{Float64}, Nonpositives{Float64}}

This is now out-of-sync because we never got to call final_touch on it.

[blegat]

Indeed, quite subtle. One solution would be to have a final_touch boolean inside StructOfConstraints and call final_touch here:

MathOptInterface.jl/src/Utilities/struct_of_constraints.jl

Line 315 in 7eb14fe

end

in case it is true.
This boolean would be initialized as false, when MOI.empty! is called it is set to false and when final_touch is called it is set to true.
This will also work with modifications. Suppose for instance that a StructOfConstraints has some fields that are MatrixOfConstraints and some that are VectorOfConstraints. For the VectorOfConstraints fields, it does support modification. Suppose you add a new constraint after the copy is done, i.e., after final_touch is called. This will not alter the boolean. If this constraint is a MatrixOfConstraints for which the field was not nothing, it will say AddConstraintNotAllowed which is expected. If this constraint is a MatrixOfConstraints for which the field was nothing, it will create a new MatrixOfConstraints and final_touch it so AddConstraintNotAllowed will be thrown as well which is consistent. For VectorOfConstraints, modification will work in both case as final_touch is a no-op anyway.

[odow]

I thought the point was that it didn't make sense to modify these after final_touch? We also have a working fix in this PR. No need for other fixes.

[blegat]

Currently, we don't have any model type for which final_touch is not a no-op and modification is supported, indeed it's a bit counter-intuitive.