Skip to content

Fix deletion of constrained variables for CachingOptimizer #905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 30, 2019

Conversation

blegat
Copy link
Member

@blegat blegat commented Sep 27, 2019

To delete constrained variables, you need to delete the whole vector of variables at once if the set dimension cannot change like MOI.SecondOrderCone. For this reason, it is important for CachingOptimizer to implement the deletion of vector of variables instead of relying on the fallback functions which deletes variable one by one otherwise you get the error:

delete_soc_variables: Error During Test at /home/blegat/.julia/dev/MathOptInterface/src/Test/config.jl:46
  Got exception outside of a @test
  MathOptInterface.DeleteNotAllowed{MathOptInterface.VariableIndex}: Deleting the index MathOptInterface.VariableIndex(1) cannot be performed: Cannot delete variable as it is constrained with other variables in a `MOI.VectorOfVariables`. You may want to use a `CachingOptimizer` in `AUTOMATIC` mode or you may need to call `reset_optimizer` before doing this operation if the `CachingOptimizer` is in `MANUAL` mode.
  Stacktrace:
   [1] throw_delete_variable_in_vov(::MathOptInterface.VariableIndex) at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/model.jl:137
   [2] _vector_of_variables_with(::Array{Tuple{MathOptInterface.ConstraintIndex{MathOptInterface.VectorOfVariables,MathOptInterface.SecondOrderCone},MathOptInterface.VectorOfVariables,MathOptInterface.SecondOrderCone},1}, ::MathOptInterface.VariableIndex) at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/model.jl:148
   [3] #152 at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/model.jl:175 [inlined]
   [4] broadcastvcat(::getfield(MathOptInterface.Utilities, Symbol("##152#154")){MathOptInterface.VariableIndex}, ::MathOptInterface.Utilities.ModelVectorConstraints{Float64,MathOptInterface.VectorOfVariables}) at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/model.jl:964
   [5] broadcastvcat(::Function, ::MathOptInterface.Utilities.Model{Float64}) at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/model.jl:935
   [6] delete(::MathOptInterface.Utilities.Model{Float64}, ::MathOptInterface.VariableIndex) at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/model.jl:175
   [7] delete(::MathOptInterface.Utilities.CachingOptimizer{MathOptInterface.AbstractOptimizer,MathOptInterface.Utilities.Model{Float64}}, ::MathOptInterface.VariableIndex) at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/cachingoptimizer.jl:375
   [8] delete at /home/blegat/.julia/dev/MathOptInterface/src/indextypes.jl:126 [inlined]
   [9] delete_soc_variables(::MathOptInterface.Utilities.CachingOptimizer{MathOptInterface.AbstractOptimizer,MathOptInterface.Utilities.Model{Float64}}, ::MathOptInterface.Test.TestConfig{Float64}) at /home/blegat/.julia/dev/MathOptInterface/src/Test/UnitTests/variables.jl:146
   [10] macro expansion at /home/blegat/.julia/dev/MathOptInterface/src/Test/config.jl:47 [inlined]
   [11] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Test/src/Test.jl:1113 [inlined]
   [12] unittest(::MathOptInterface.Utilities.CachingOptimizer{MathOptInterface.AbstractOptimizer,MathOptInterface.Utilities.Model{Float64}}, ::MathOptInterface.Test.TestConfig{Float64}, ::Array{String,1}) at /home/blegat/.julia/dev/MathOptInterface/src/Test/config.jl:47
   [13] unittest(::MathOptInterface.Utilities.CachingOptimizer{MathOptInterface.AbstractOptimizer,MathOptInterface.Utilities.Model{Float64}}, ::MathOptInterface.Test.TestConfig{Float64}) at /home/blegat/.julia/dev/MathOptInterface/src/Test/config.jl:42
   [14] top-level scope at /home/blegat/.julia/dev/MathOptInterface/test/Utilities/cachingoptimizer.jl:419
   [15] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Test/src/Test.jl:1113
   [16] top-level scope at /home/blegat/.julia/dev/MathOptInterface/test/Utilities/cachingoptimizer.jl:419
   [17] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Test/src/Test.jl:1186
   [18] top-level scope at /home/blegat/.julia/dev/MathOptInterface/test/Utilities/cachingoptimizer.jl:393

@codecov-io
Copy link

codecov-io commented Sep 27, 2019

Codecov Report

Merging #905 into master will increase coverage by 0.02%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #905      +/-   ##
==========================================
+ Coverage   95.12%   95.15%   +0.02%     
==========================================
  Files          81       81              
  Lines        8592     8662      +70     
==========================================
+ Hits         8173     8242      +69     
- Misses        419      420       +1
Impacted Files Coverage Δ
src/Test/UnitTests/variables.jl 88.19% <100%> (+6.87%) ⬆️
src/Utilities/cachingoptimizer.jl 92.91% <88.23%> (-0.36%) ⬇️
src/Utilities/mockoptimizer.jl 90.33% <0%> (+0.48%) ⬆️

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 0e11c54...0060461. Read the comment docs.

@blegat blegat added this to the v0.9.4 milestone Sep 28, 2019
@test !MOI.is_valid(model, cv)
v, cv = MOI.add_constrained_variables(model, MOI.SecondOrderCone(3))
@test MOI.get(model, MOI.NumberOfVariables()) == 3
@test_throws MOI.DeleteNotAllowed MOI.delete(model, v[1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm.... I hadn't thought of this case. I guess it's reasonable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing existing tests with bridged variables put a lot of stress on the deletion API so as part of adding the variable bridges, we clarified what should be done in these corner cases. The relevant PR is #793

@blegat blegat merged commit 85f23cd into master Sep 30, 2019
@blegat blegat deleted the bl/del_constr_vars_cachopt branch October 10, 2019 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants