Tests for vector specialized version of MOI.delete

[henriquebecker91]

Contribution based in a discussion with @odow and @mlubin. I think the tests may cover more cases but will wait suggestions. Looking at the other tests seem like the style of choice is keeping things simple.

The gist of this contribution is that MOI.delete taking a vector may be specialized to be 'smarter' (i.e., not O(n^2) in some situations as it is now) but this 'smarter' code needs extra tests.

[codecov-io]

Codecov Report

Merging #989 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #989      +/-   ##
==========================================
+ Coverage   95.25%   95.26%   +<.01%     
==========================================
  Files          97       97              
  Lines       10821    10838      +17     
==========================================
+ Hits        10308    10325      +17     
  Misses        513      513

Impacted Files	Coverage Δ
src/Test/UnitTests/modifications.jl	100% <100%> (ø)	⬆️

---

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 b55cc2e...7f6d4cc. Read the comment docs.

[mlubin]

Thanks! Did you check that Gurobi and CPLEX pass this test with jump-dev/Gurobi.jl#282 and jump-dev/CPLEX.jl#273?

[henriquebecker91]

I did test with both Gurobi.jl and CPLEX.jl, both worked when I used their branchs merged with the most recent commit in their masters against MOI 0.9.8 (the master from one, or two, weeks ago). When I updated MOI for the most recent commit, Gurobi.jl tests kept working and CPLEX.jl tests stopped working for unrelated reasons (i.e., running runtests.jl with most recent MOI and CPLEX.jl master commits gives to me a "UndefVarError: AddSubMul not defined" error).

[odow]

Can you add tests for this please. Basically, add

@testset "delete_variables_in_a_batch" begin
        MOIU.set_mock_optimize!(mock,
            (mock::MOIU.MockOptimizer) -> MOIU.mock_optimize!(mock,
                MOI.OPTIMAL, (MOI.FEASIBLE_POINT, [1.0, 1.0, 1.0]),
                MOI.FEASIBLE_POINT
            ),
            (mock::MOIU.MockOptimizer) -> MOIU.mock_optimize!(mock,
                MOI.OPTIMAL, (MOI.FEASIBLE_POINT, [1.0]),
                MOI.FEASIBLE_POINT
            )
        )
        MOIT.delete_variables_in_a_batch(mock, config)
    end

To this testset:
https://github.com/JuliaOpt/MathOptInterface.jl/blob/b123627c0589d291e466ccaa5a00db93dcca4a3d/test/Test/unit.jl#L397-L413

[henriquebecker91]

I added what you asked @odow.

I updated the dependencies of MOI to the versions used in MOI@0.9.9 (when I started to implement this PR the version was 0.9.8) and fixed the "UndefVarError: AddSubMul not defined" error (the problem was I was not using the last version of MutableArithmetics). I executed the 'runtests.jl' of both CPLEX.jl and Gurobi.jl and all tests related to MathOptInterface passed. In my local copy I added some 'error' calls to the MOI tests added and to the MOI.delete specializations (inside CPLEX.jl and Gurobi.jl) so to confirm they are being called. They are.

Is some benchmark test needed to check how much difference is the performance of the specializations against the fallback?

[odow]

No need for benchmark. This is just to check that if solvers implement the vector-valued version, that they haven't made trivial mistakes.

There is certainly room for additional testing with edge cases if you have time and the inclination to come up with them, but it's hard to test every case. I don't think that's needed for now though.

[henriquebecker91]

Do you want me to come with more cases before you merge? I really agree that more tests is better, but unfortunately I need to focus on other things now (PhD is ticking), if it is really necessary I can try to write some extra tests before going back to my things.

[odow]

This test is fine. It catches the most common error of non-contiguous variables being deleted.