Support of bridges with external sets/funcs #510

IssamT · 2018-08-27T18:58:27Z

Simpler solution as suggested by @blegat. Of course we can now delete _mod function and get rid of its calls.

However even with this solution, we still need to import sets/functions to be used by @bridge

If we want the modules passed to @bridge like MOI.xxxfunction/MOI.xxxset, we need more modifications in @model since it uses the names passed to create field names of the new structure. Do we go for that?

blegat · 2018-08-27T19:26:46Z

Yes, you can create unique field names with gensym. The names of the fields does not matter too much, I have chosen to make them match the name of the sets/functions initially because it was simple to do but here suppose two different modules have the same function names, you will generate two fields with the same name. It is better to change to using gensym IMO.

blegat · 2018-08-27T19:29:22Z

src/Bridges/singlebridgeoptimizer.jl

@@ -45,7 +45,7 @@ macro bridge(modelname, bridge, ss, sst, vs, vst, sf, sft, vf, vft)
    esc(quote
        $MOIU.@model $bridged_model_name $ss $sst $vs $vst $sf $sft $vf $vft
        const $modelname{T, OT<:MOI.ModelLike} = $MOIB.SingleBridgeOptimizer{$bridge{T}, $bridged_model_name{T}, OT}
-        is_bridged(::$modelname, ::Type{<:$bridged_funs}, ::Type{<:$bridged_sets}) = true
+        MathOptInterface.Bridges.is_bridged(::$modelname, ::Type{<:$bridged_funs}, ::Type{<:$bridged_sets}) = true


You can use $MOIB.is_bridged

IssamT · 2018-08-27T21:53:34Z

If we want the modules passed to @bridge like MOI.xxxfunction/MOI.xxxset, we need more modifications in @model since it uses the names passed to create field names of the new structure.

I guess I should have tested first. I was surprised that this is not needed and the reason is:

julia> struct abc
           my.name::String
       end
ERROR: syntax: field name "my.name" is not a symbol

As Expected. But using metaprogramming you can put a dot inside the name of a field, although it s not clean at all.

julia> s = Symbol("my.name")
Symbol("my.name")

julia> code = quote
              struct abc
                 $(s)::String
              end
              end
quote
    #= REPL[24]:2 =#
    struct abc
        #= REPL[24]:3 =#
        my.name::String
    end
end

julia> eval(code)

julia> abc("Bob")
abc("Bob")

Maybe I will transform it to underscore to keep readable names? gensym will make debugging hard. No?

codecov-io · 2018-08-27T23:09:08Z

Codecov Report

Merging #510 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
+ Coverage   95.59%   95.62%   +0.02%     
==========================================
  Files          46       46              
  Lines        4700     4704       +4     
==========================================
+ Hits         4493     4498       +5     
+ Misses        207      206       -1

Impacted Files	Coverage Δ
src/Bridges/Bridges.jl	`100% <ø> (ø)`	⬆️
src/Bridges/singlebridgeoptimizer.jl	`100% <100%> (+14.28%)`	⬆️
src/Utilities/model.jl	`100% <100%> (ø)`	⬆️
src/Utilities/cachingoptimizer.jl	`89.94% <0%> (-0.11%)`	⬇️
src/Utilities/copy.jl	`93.82% <0%> (+0.07%)`	⬆️
src/attributes.jl	`97.05% <0%> (+0.39%)`	⬆️

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 c8e597e...9fcbb3f. Read the comment docs.

blegat · 2018-08-28T06:33:42Z

@IssamT The fields are not shown in printing so I have never seen the fields in fact. If you can make it work with underscores, it is fine too.

blegat · 2018-08-28T06:36:57Z

src/Utilities/model.jl

 # (:Zeros) -> :(MOI.Zeros)
-_moi(s::Symbol) = _mod(MOI, s)
+_moi(s::Union{Symbol,Expr}) = _mod(MOI, s)
 _set(s::SymbolSet) = _moi(s.s)


Let's remove _moi and _mod

blegat

Looks good, thanks ! :)

odow · 2018-08-28T10:53:24Z

src/Utilities/model.jl

@@ -393,12 +393,12 @@ abstract type Constraints{F} end

 abstract type SymbolFS end
 struct SymbolFun <: SymbolFS
-    s::Symbol
+    s::Union{Symbol,Expr}


Nit: missing a space Union{Symbol, Expr}

odow · 2018-08-28T10:54:02Z

src/Utilities/model.jl

@@ -579,14 +577,14 @@ macro model(modelname, ss, sst, vs, vst, sf, sft, vf, vft)
    end

    for (func, T) in ((:_add_constraint, CI), (:_modify, CI), (:_delete, CI), (:_getindex, CI), (:_getfunction, CI), (:_getset, CI), (:_getnoc, MOI.NumberOfConstraints))
-        funct = _mod(MOIU, func)
+        funct = func


Why have this extra variable?

odow · 2018-08-28T10:54:32Z

src/Utilities/model.jl

-
-    end)
+    end
+    esc(code)


Style: missing a return

odow · 2018-08-28T10:55:21Z

test/runtests.jl

-               (VectorAffineFunction, VectorQuadraticFunction))
+               (MOI.ZeroOne, MOI.Integer),
+               (MOI.EqualTo, MOI.GreaterThan, MOI.LessThan, MOI.Interval, MOI.Semicontinuous, MOI.Semiinteger),
+               (MOI.Reals, MOI.Zeros, MOI.Nonnegatives, MOI.Nonpositives, MOI.SecondOrderCone, MOI.RotatedSecondOrderCone, MOI.GeometricMeanCone, MOI.ExponentialCone, MOI.DualExponentialCone, MOI.PositiveSemidefiniteConeTriangle, MOI.PositiveSemidefiniteConeSquare, MOI.RootDetConeTriangle, MOI.RootDetConeSquare, MOI.LogDetConeTriangle, MOI.LogDetConeSquare),


Style: wrap really long lines? (Lots of places with this in the model definitions.)

blegat · 2018-08-28T11:49:24Z

src/Bridges/singlebridgeoptimizer.jl

@@ -17,7 +17,7 @@ is_bridged(b::SingleBridgeOptimizer, ::Type{<:MOI.AbstractFunction}, ::Type{<:MO
 bridge_type(b::SingleBridgeOptimizer{BT}, F::Type{<:MOI.AbstractFunction}, S::Type{<:MOI.AbstractSet}) where BT = BT

 # :((Zeros, SecondOrderCone)) -> (:(MOI.Zeros), :(MOI.SecondOrderCone))
-_tuple_prefix_moi(t) = MOIU._moi.(t.args)
+_tuple_prefix_moi(t) = t.args


It seems the function is not used anymore, let's remove it

mlubin

I have some ideas to improve the syntax of these macros, but that can happen later.

Support of bridges with external sets/funcs

591072b

IssamT mentioned this pull request Aug 27, 2018

Support to bridges with external sets/functions #490

Closed

blegat reviewed Aug 27, 2018

View reviewed changes

improvement and tests updated

56ef4fd

blegat reviewed Aug 28, 2018

View reviewed changes

blegat approved these changes Aug 28, 2018

View reviewed changes

removed _mod and _moi

bcb5502

odow reviewed Aug 28, 2018

View reviewed changes

blegat reviewed Aug 28, 2018

View reviewed changes

IssamT added 2 commits August 28, 2018 14:51

fixing style

ae6fdcf

deleting useless var

f059cae

mlubin approved these changes Aug 28, 2018

View reviewed changes

one more line wrapped

9fcbb3f

blegat merged commit a2aa54e into jump-dev:master Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support of bridges with external sets/funcs #510

Support of bridges with external sets/funcs #510

Uh oh!

IssamT commented Aug 27, 2018

Uh oh!

blegat commented Aug 27, 2018 •

edited

Loading

Uh oh!

blegat Aug 27, 2018 •

edited

Loading

Uh oh!

IssamT commented Aug 27, 2018 •

edited

Loading

Uh oh!

codecov-io commented Aug 27, 2018 •

edited

Loading

Uh oh!

blegat commented Aug 28, 2018

Uh oh!

blegat Aug 28, 2018

Uh oh!

blegat left a comment

Uh oh!

odow Aug 28, 2018

Uh oh!

odow Aug 28, 2018

Uh oh!

odow Aug 28, 2018

Uh oh!

odow Aug 28, 2018

Uh oh!

blegat Aug 28, 2018

Uh oh!

mlubin left a comment

Uh oh!

Uh oh!

Support of bridges with external sets/funcs #510

Support of bridges with external sets/funcs #510

Uh oh!

Conversation

IssamT commented Aug 27, 2018

Uh oh!

blegat commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blegat Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IssamT commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blegat commented Aug 28, 2018

Uh oh!

blegat Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

blegat left a comment

Choose a reason for hiding this comment

Uh oh!

odow Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

odow Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

odow Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

odow Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

blegat Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

mlubin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blegat commented Aug 27, 2018 •

edited

Loading

blegat Aug 27, 2018 •

edited

Loading

IssamT commented Aug 27, 2018 •

edited

Loading

codecov-io commented Aug 27, 2018 •

edited

Loading