fix ambiguity by matbesancon · Pull Request #748 · jump-dev/MathOptInterface.jl

matbesancon · 2019-05-23T09:22:59Z

There was a method ambiguity because of the non-sub-typing in the indicator bridge definition.

matbesancon · 2019-05-23T09:24:12Z

I caught this with SCIP:

 Resolving package versions...
Test Summary:        | Pass  Total
direct library calls |   19     19
Test Summary:  | Pass  Total
managed memory |   93     93
indicator3: Error During Test at /home/mbesancon/.julia/packages/MathOptInterface/XjmMj/src/Test/config.jl:45
  Got exception outside of a @test
  MethodError: MathOptInterface.Bridges.concrete_bridge_type(::Type{MathOptInterface.Bridges.IndicatorActiveOnFalseBridge{Float64,MathOptInterface.VectorAffineFunction{Float64},MathOptInterface.LessThan{Float64}}}, ::Type{MathOptInterface.VectorAffineFunction{Float64}}, ::Type{MathOptInterface.IndicatorSet{ACTIVATE_ON_ZERO::ActivationCondition = 0,MathOptInterface.LessThan{Float64}}}) is ambiguous. Candidates:
    concrete_bridge_type(::Type{#s640} where #s640<:(MathOptInterface.Bridges.IndicatorActiveOnFalseBridge{T,F,S} where S<:MathOptInterface.AbstractScalarSet where F<:MathOptInterface.AbstractVectorFunction), ::Type{F}, ::Type{MathOptInterface.IndicatorSet{ACTIVATE_ON_ZERO::ActivationCondition = 0,S}}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/packages/MathOptInterface/XjmMj/src/Bridges/indicatorbridge.jl:56
    concrete_bridge_type(bridge_type::DataType, ::Type{#s640} where #s640<:MathOptInterface.AbstractFunction, ::Type{#s639} where #s639<:MathOptInterface.AbstractSet) in MathOptInterface.Bridges at /home/mbesancon/.julia/packages/MathOptInterface/XjmMj/src/Bridges/bridge.jl:93
  Possible fix, define
    concrete_bridge_type(::Type{#s640} where #s640<:(MathOptInterface.Bridges.IndicatorActiveOnFalseBridge{T,F,S} where S<:MathOptInterface.AbstractScalarSet where F<:MathOptInterface.AbstractVectorFunction), ::Type{F<:MathOptInterface.AbstractFunction}, ::Type{MathOptInterface.IndicatorSet{ACTIVATE_ON_ZERO::ActivationCondition = 0,S}})
  Stacktrace:
   [1] concrete_bridge_type(::MathOptInterface.Bridges.LazyBridgeOptimizer{SCIP.Optimizer}, ::Type{MathOptInterface.VectorAffineFunction{Float64}}, ::Type{MathOptInterface.IndicatorSet{ACTIVATE_ON_ZERO::ActivationCondition = 0,MathOptInterface.LessThan{Float64}}}) at /home/mbesancon/.julia/packages/MathOptInterface/XjmMj/src/Bridges/bridgeoptimizer.jl:59
   [2] add_constraint(::MathOptInterface.Bridges.LazyBridgeOptimizer{SCIP.Optimizer}, ::MathOptInterface.VectorAffineFunction{Float64}, ::MathOptInterface.IndicatorSet{ACTIVATE_ON_ZERO::ActivationCondition = 0,MathOptInterface.LessThan{Float64}}) at /home/mbesancon/.julia/packages/MathOptInterface/XjmMj/src/Bridges/bridgeoptimizer.jl:447

matbesancon · 2019-05-23T09:39:54Z

For info, other ambiguities are present:

julia> Test.detect_ambiguities(MathOptInterface , imported=false)
4-element Array{Tuple{Method,Method},1}:
 (get(b::MathOptInterface.Bridges.VectorSlackBridge{T,F,S}, ::MathOptInterface.ListOfConstraintIndices{F,MathOptInterface.Zeros}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/dev/MathOptInterface/src/Bridges/slackbridge.jl:155, get(b::MathOptInterface.Bridges.VectorSlackBridge{T,F,S}, ::MathOptInterface.ListOfConstraintIndices{MathOptInterface.VectorOfVariables,S}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/dev/MathOptInterface/src/Bridges/slackbridge.jl:156)
 (get(b::MathOptInterface.Bridges.ScalarSlackBridge{T,F,S}, ::MathOptInterface.ListOfConstraintIndices{F,MathOptInterface.EqualTo{T}}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/dev/MathOptInterface/src/Bridges/slackbridge.jl:53, get(b::MathOptInterface.Bridges.ScalarSlackBridge{T,F,S}, ::MathOptInterface.ListOfConstraintIndices{MathOptInterface.SingleVariable,S}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/dev/MathOptInterface/src/Bridges/slackbridge.jl:54)
 (get(b::MathOptInterface.Bridges.VectorSlackBridge{T,F,S}, ::MathOptInterface.NumberOfConstraints{F,MathOptInterface.Zeros}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/dev/MathOptInterface/src/Bridges/slackbridge.jl:153, get(b::MathOptInterface.Bridges.VectorSlackBridge{T,F,S}, ::MathOptInterface.NumberOfConstraints{MathOptInterface.VectorOfVariables,S}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/dev/MathOptInterface/src/Bridges/slackbridge.jl:154)        
 (get(b::MathOptInterface.Bridges.ScalarSlackBridge{T,F,S}, ::MathOptInterface.NumberOfConstraints{F,MathOptInterface.EqualTo{T}}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/dev/MathOptInterface/src/Bridges/slackbridge.jl:51, get(b::MathOptInterface.Bridges.ScalarSlackBridge{T,F,S}, ::MathOptInterface.NumberOfConstraints{MathOptInterface.SingleVariable,S}) where {T, F, S} in MathOptInterface.Bridges at /home/mbesancon/.julia/dev/MathOptInterface/src/Bridges/slackbridge.jl:52)

codecov-io · 2019-05-23T09:45:14Z

Codecov Report

Merging #748 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #748      +/-   ##
==========================================
+ Coverage   94.26%   94.31%   +0.05%     
==========================================
  Files          56       56              
  Lines        6170     6211      +41     
==========================================
+ Hits         5816     5858      +42     
+ Misses        354      353       -1

Impacted Files	Coverage Δ
src/Bridges/indicatorbridge.jl	`100% <100%> (ø)`	⬆️
src/Bridges/rsocbridge.jl	`100% <0%> (ø)`	⬆️
src/Utilities/cachingoptimizer.jl	`92.48% <0%> (ø)`	⬆️
src/Test/UnitTests/basic_constraint_tests.jl	`97.61% <0%> (ø)`	⬆️
src/attributes.jl	`94.66% <0%> (ø)`	⬆️
src/Utilities/functions.jl	`95% <0%> (+0.07%)`	⬆️
src/Bridges/bridgeoptimizer.jl	`91.82% <0%> (+1.82%)`	⬆️

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 6cbd7a8...1bf0674. Read the comment docs.

blegat · 2019-05-23T19:37:38Z

For info, other ambiguities are present:

They should not happen, e.g. because slack bridges do not bridge SingleVariable-in-EqualTo constraints. Thanks for the info, I didn't know about this detect_ambiguities

blegat · 2019-05-23T19:43:20Z

We are missing a tests. I suggest to do these changes:

The bridge type should be concrete here:
https://github.com/JuliaOpt/MathOptInterface.jl/pull/748/files#diff-9b669e0d032ef727dedac270185d6451R18

And concrete_bridge_type should be called here:
https://github.com/JuliaOpt/MathOptInterface.jl/pull/748/files#diff-9b669e0d032ef727dedac270185d6451R18

matbesancon · 2019-05-24T09:41:36Z

The bridge type should be concrete here

Why? It is accepting a sub-type of IndicatorActiveOnFalseBridge so will remain valid

blegat · 2019-05-24T12:14:17Z

Why?

We always give a concrete type to this function:
https://github.com/JuliaOpt/MathOptInterface.jl/blob/06216dc6907b5ec76390c8380e4655065c83ea22/src/Bridges/bridgeoptimizer.jl#L455-L461
and the first argument is always a concrete type for other bridges. By consistency, it would be better if we do the same for the indicator bridge.

matbesancon · 2019-05-24T13:21:10Z

Fixed in last commit, it calls concrete_bridge_type

blegat · 2019-05-24T15:13:48Z

You didn't push the commit

matbesancon · 2019-05-24T15:22:54Z

Last commit from 6 hours ago, calls concrete_bridge_type

blegat · 2019-05-24T15:42:11Z

It should be called in the tests and the methods should only be defined when the first argument is concrete

matbesancon · 2019-05-24T22:01:38Z

I won't have time for this before at least sunday evening I think

…

On Fri, May 24, 2019 at 5:42 PM Benoît Legat ***@***.***> wrote: It should be called in the tests and the methods should only be defined when the first argument is concrete — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#748>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB2FDMTJ3BJFH5X7ZYDCJNDPXAEFJANCNFSM4HO3V67Q> .

-- Mathieu Besançon

blegat · 2019-05-25T08:52:03Z

No rush, MOI v0.9 won't be tagged with only 2 solvers ready: #736

matbesancon · 2019-05-28T09:57:30Z

ok, tests pass, and SCIP tests also with this version

matbesancon · 2019-05-28T14:53:01Z

@blegat seems good here

fix ambiguity

8aacba9

use concrete bridge type

5720668

blegat added this to the v0.9 milestone May 25, 2019

matbesancon added 2 commits May 28, 2019 10:14

concrete bridge tests

1b55be7

fix subtyping

9c00b02

blegat reviewed May 28, 2019

View reviewed changes

Comment thread src/Bridges/indicatorbridge.jl Outdated

matbesancon added 3 commits May 28, 2019 13:51

bridge constraint concrete type

a9853a4

fix test

c6eea12

remove BT

9c03f61

blegat reviewed May 28, 2019

View reviewed changes

Comment thread src/Bridges/indicatorbridge.jl Outdated

subtype directly in-place

1bf0674

blegat approved these changes May 28, 2019

View reviewed changes

blegat merged commit 52d166f into jump-dev:master May 28, 2019

Conversation

matbesancon commented May 23, 2019

Uh oh!

matbesancon commented May 23, 2019

Uh oh!

matbesancon commented May 23, 2019

Uh oh!

codecov-io commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blegat commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blegat commented May 23, 2019

Uh oh!

matbesancon commented May 24, 2019

Uh oh!

blegat commented May 24, 2019

Uh oh!

matbesancon commented May 24, 2019

Uh oh!

blegat commented May 24, 2019

Uh oh!

matbesancon commented May 24, 2019

Uh oh!

blegat commented May 24, 2019

Uh oh!

matbesancon commented May 24, 2019 via email

Uh oh!

blegat commented May 25, 2019

Uh oh!

matbesancon commented May 28, 2019

Uh oh!

Uh oh!

matbesancon commented May 28, 2019

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

codecov-io commented May 23, 2019 •

edited

Loading

blegat commented May 23, 2019 •

edited

Loading