Adds buffering for variables deleted.

[henriquebecker91]

Gurobi.Optimizer now have fields next_column and columns_deleted_since_last_update. The behavior now is to not force a update before deleting, but instead: update the list of variables in Julia side but not their column indexes, and save the column indexes of the ones deleted; when updating the model then update the column indexes in Julia side.

The code pass all tests. But maybe we need to devise new ones.

This fix #315 in my code (that unfortunately is not a MWE).

I will be out for about 2 hours now, @odow, after this I will check if you have suggested something to me to improve in this PR.

[odow]

We definitely need some tests for this one.

My example from this issue should be fine.

[henriquebecker91]

`deepcopy` is not needed per se, is just an habit of mine to not assume fields will not be changed to mutable types after. It should be compiled to a no-operation, but I can remove it if you think it is better. I will make your example a test then. Just not now. Em seg., 4 de mai. de 2020 às 16:13, Oscar Dowson <notifications@github.com> escreveu:

…

***@***.**** commented on this pull request. We definitely need some tests for this one. My example from this issue should be fine. ------------------------------ In src/MOI_wrapper.jl <#317 (comment)>: > @@ -419,6 +453,12 @@ function _info(model::Optimizer, key::MOI.VariableIndex) throw(MOI.InvalidIndex(key)) end +function _get_next_column(model::Optimizer) + old_value = deepcopy(model.next_column) Why is deepcopy needed? ------------------------------ In src/MOI_wrapper.jl <#317 (comment)>: > @@ -419,6 +453,12 @@ function _info(model::Optimizer, key::MOI.VariableIndex) throw(MOI.InvalidIndex(key)) end +function _get_next_column(model::Optimizer) + old_value = deepcopy(model.next_column) + model.next_column += 1 + return old_value function _get_next_column(model::Optimizer) model.next_column += 1 return model.next_column - 1end — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#317 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLVVGYZSGBHF2DOCCNAVD3RP4HWRANCNFSM4MZAGVKA> .

[henriquebecker91]

@odow The test was a little harder to create than I expected. Many common attributes are cached on the wrapper (name, bounds, type, ...) and other are discarded (Start, VBasis, ...) if any variables are deleted. This explains how this bug did not surface immediately, and how the code seems to work in some situations that it shouldn't work, given the nature of the bug.

The testset has 4 fails in current master branch and none with the changes introduced by this branch. I think it is a solid test.

[odow]

Which are the tests that fail?

[henriquebecker91]

Buffered deletion test: Test Failed at /home/henrique/AreaDeTrabalho/Gurobi.jl/test/MOI/MOI_wrapper.jl:519
  Expression: vars_obj[2] == MOI.get(model, obj_attr, vars[2])
   Evaluated: 42.0 == -77.0
Buffered deletion test: Test Failed at /home/henrique/AreaDeTrabalho/Gurobi.jl/test/MOI/MOI_wrapper.jl:521
  Expression: fourth_var_obj == MOI.get(model, obj_attr, fourth_var)
   Evaluated: -77.0 == 0.0
Buffered deletion test: Test Failed at /home/henrique/AreaDeTrabalho/Gurobi.jl/test/MOI/MOI_wrapper.jl:526
  Expression: vars_obj[2] == MOI.get(model, obj_attr, vars[2])
   Evaluated: 42.0 == -77.0
Buffered deletion test: Test Failed at /home/henrique/AreaDeTrabalho/Gurobi.jl/test/MOI/MOI_wrapper.jl:528
  Expression: fourth_var_obj == MOI.get(model, obj_attr, fourth_var)
   Evaluated: -77.0 == 0.0

Basically the tests that check the value of the property set. The ones before the implicit update_model! caused by MOI.optimize! and the ones after it. In the patched version such errors do not exist (all tests pass). So I am confident the code, while not bug-free, solves the bug it intends to solve and do not break any other test.

[odow]

Oops. I mis-read your comment. I understood it as 4 different tests were failing, but these ones were passing.

[henriquebecker91]

Oh, no. With this PR all older tests pass and the new ones too. In the master branch, the 4 @test I cited of the new @testset I created fail (because the bug the PR solves).

[henriquebecker91]

@odow, do you want me to improve this PR in some way? While this is not approved, Gurobi.jl will cause subtle bugs to users when deletion is followed by attribute changes. It is also good for me if I can refer to the Gurobi version I used in my paper, instead of saying I used a patched local copy, XD.

[odow]

Sorry, merging since this is a strict improvement in correcting an existing bug. Thanks!