Create a version of method delete that deletes a vector of variables.

[henriquebecker91]

This calls the version of MOI.delete that also works over a vector of variables, and is specialized by solvers for greater performance than repeatedly calling the single variable version.

The code was suggested by @odow in a discussion with him and @mlubin.

This is related to:
jump-dev/MathOptInterface.jl#989
jump-dev/Gurobi.jl#282
jump-dev/CPLEX.jl#273

The CPLEX and Gurobi PR's are already closed, the MathOptInterface is just waiting more reviewers (I think). Anyway, MathOptInterface already has a "delete a vector of variables" fallback method (https://github.com/JuliaOpt/MathOptInterface.jl/blob/c4beceb4e9444c752f7e655f7a50a38a70181b06/src/indextypes.jl#L124-L128). So this addition to JuMP would already be working if it was approved immediately. I suppose tests may be necessary, but it is MOI that does the heavy lifting and this method is basically the same as the single variable version except by the type of the parameters and the version of the MOI method it calls.

I imported the JuMP package changed by my branch in the REPL and called the method to test against typos.

[odow]

Can you add a test to test/variables.jl please? Can just be the same one as the one you added to MOI (but with JuMP syntax).

[henriquebecker91]

Will do it.

[codecov]

Codecov Report

Merging #2135 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2135      +/-   ##
==========================================
+ Coverage    91.4%   91.42%   +0.01%     
==========================================
  Files          42       42              
  Lines        4097     4105       +8     
==========================================
+ Hits         3745     3753       +8     
  Misses        352      352

Impacted Files	Coverage Δ
src/variables.jl	95.87% <100%> (+0.08%)	⬆️
src/constraints.jl	91.85% <100%> (+0.24%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a656f6...30d6589. Read the comment docs.

[henriquebecker91]

Added the tests and changed JuMPExtensions to implement the batch delete and pass the tests too. The tests are similar to the ones for a single variable in JuMP and to the ones over multiple variables in MOI.

I think it is now ready for review.

[mlubin]

Awesome!

[mlubin]

Users will wonder why we have JuMP.delete for vectors of variables but not vectors of constraints (MOI.delete supports vectors of constraints). We could add this now or leave a TODO in the appropriate spot in the code.

[henriquebecker91]

I will implement the batch constraint deletion in JuMP too. However, I think it would be good to keep in mind that:

1. I do not know if there are advantages for batch constraint deletion. Probably there are, I just really never checked.
2. I already wondered why there was not a batch deletion method for variables without existing one for constraints or something similar, XD.
3. I am a little worried that having a batch deletion of constraints will lure the users to think this has, or will have in the close future, a performance boost, when we do not have immediate plans for that nor are sure there will be such gain. On the plus side, if there is a gain and it is implemented in the future, the sooner we have such method, the more users that will use it without thinking and will be positively affected when it is specialized.
4. I will:
   4.1) Implement a trivial fallback delete. Make the same comment about performance I did in the batch variable delete version.
   4.2) Do the same for JuMPExtensions.
   4.3) Add tests for this method to be executed over both a JuMP model and a JuMPExtensions model.

[henriquebecker91]

I implemented the items 4.1, 4.2, and 4.3 from my last comment.

To make it work, I defined the method signature as:
function delete(model::Model, con_refs::Vector{<:ConstraintRef{Model}})
instead of
function delete(model::Model, con_refs::Vector{ConstraintRef{Model}})
that was my first try. I am not sure if what I did is correct (I have not a good mental model for how these types should interact in JuMP design). Should I change the variable deletion methods to work in subtypes too? Or should I make the code work without accepting constraint reference subtypes in the constraint deletion methods ? (The tests fail with the second signature, not sure why.) Or only the constraint deletion methods need to work on subtypes (like it is now) and variable deletion should also stay the way it is now?

[odow]

ConstraintRef has multiple type parameters:
https://github.com/JuliaOpt/JuMP.jl/blob/7a656f69cebaa718edba061523281007304fd0cc/src/constraints.jl#L16-L20
whereas VariableRef doesn't have any
https://github.com/JuliaOpt/JuMP.jl/blob/7a656f69cebaa718edba061523281007304fd0cc/src/variables.jl#L105-L108

An easier way to reason about this is that [1, 2] isa Vector{Int} == true, [1, 2] isa Vector{Integer} == false, and [1, 2] isa Vector{<:Integer} == true. The second case is saying "is this a vector with eltype exactly Integer?" To which the answer is no, because Int != Integer. However, the third case is saying "is this a vector with eltype that is a subtype of Integer?" To which the answer is true because Int <: Integer.

So Vector{VariableRef} is okay, but Vector{ConstraintRef} isn't, because it's really Vector{ConstraintRef{M, C, Shape} where {M, C, Shape}}. You need Vector{<:ConstraintRef}.

[henriquebecker91]

Thanks for the explanation, @odow. Taking this into account, then there is no change to be done in the signature of the methods.