Add the constructvariable!/variabletype functions #1029

blegat · 2017-05-16T22:11:45Z

This allows extensions to use the @variable macro to create other types
of variables. For example, in PolyJuMP, we could add the syntax

@variable(m, p, Poly(X))
@variable(m, p >= 0, Poly(X))
# or
@variable(m, p, :Poly, monomials=X)
@variable(m, p >= 0, :Poly, monomials=X)

where X is the vector of monomials

@variable

This allows extensions to use the @variable macro to create other types of variables

joehuchette · 2017-05-16T22:29:34Z

Cool. I like the first syntax FWIW. Could also be @variable(m, p, Poly(monomials=X)).

mlubin · 2017-05-17T00:10:07Z

It's hard to keep track of what the @variable macro does at this point. I think we need to fully document what syntax the @variable macro supports before we can decide about extending it. Sorry to ask you to do more work, @blegat, but as we push to 1.0 we need to make sure that currently messy parts of the code like @variable become more well thought out, consistent, and easy to explain.

odow · 2017-05-17T01:09:40Z

I think this needs to be done in conjunction with #692

@variable(m, x[i,j] in Int for i in I, j in J if cond(i,j))
@variable(m, x[i,j] in [l, u] for i in I, j in J if cond(i,j))
@variable(m, x[i,j] in {0} | [l, u] for i in I, j in J if cond(i,j))
@variable(m, x[i,j] >= 0 in Poly(X) for i in I, j in J if cond(i,j))

mlubin · 2017-05-17T01:14:46Z

#692 is going to need a discussion at the developers meetup, and I'm not super convinced about it.
I'm open to non-breaking or incremental changes to the current syntax, but it needs to be well thought out and documented.

codecov · 2017-05-17T13:17:14Z

Codecov Report

Merging #1029 into master will decrease coverage by 1.98%.
The diff coverage is 88.37%.

@@            Coverage Diff             @@
##           master    #1029      +/-   ##
==========================================
- Coverage   90.56%   88.57%   -1.99%     
==========================================
  Files          18       18              
  Lines        4610     4500     -110     
==========================================
- Hits         4175     3986     -189     
- Misses        435      514      +79

Impacted Files	Coverage Δ
src/macros.jl	`86.89% <88.37%> (-4.46%)`	⬇️
src/parseExpr_staged.jl	`81.55% <0%> (-11.23%)`	⬇️
src/parsenlp.jl	`61.53% <0%> (-7.22%)`	⬇️
src/affexpr.jl	`88.31% <0%> (-2.6%)`	⬇️
src/operators.jl	`90.14% <0%> (-2.09%)`	⬇️
src/callbacks.jl	`84.86% <0%> (-1.11%)`	⬇️
src/deprecated.jl	`0% <0%> (ø)`	⬆️
src/JuMP.jl	`91.74% <0%> (+0.41%)`	⬆️

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 1c82cf8...b5f3e1c. Read the comment docs.

blegat · 2017-05-17T13:28:46Z

I have added a developer documentation of @variable in 22bc8fe. I agree that transforming Bin, Int, ... before passing them to constructvariable! is a bit hacky since we do not apply the same treatments to the other elements of extra. As a non-breaking change, we could define the types Bin, SemiCont, ... however this may also look strange...

@odow I also like the in syntax but I have tried to make this PR as small as possible since it is already an arguable change. I was thinking to add the second argument of in at the beginning of extra so that @variable(m, x[i,j] in Int) would be equivalent to @variable(m, x[i,j], Int) but that would belong to a different PR.

mlubin · 2017-05-17T14:15:10Z

src/macros.jl

+#   passed to `constructvariable!`.
+# * The keyword arguments start, objective, inconstraints, coefficients, basename, lowerbound, upperbound, category may not be passed as is to
+#   `constructvariable!` since they may be altered by the parsing of `expr` and we may need to pass it pointwise if it is a container since
+#   `constructvariable!` is called separately for each variable of the container.


Could you give a trivial, e.g. @variable(m, x >= 0) and nontrivial example, e.g., @variable(m, x[1:N,1:N], SDP, Poly()) of how the macro creates calls to constructvariable!?

mlubin · 2017-05-17T14:15:55Z

src/macros.jl

+
+variabletype(m::Model, t::Symbol) = variabletype(m, Val{t})
+function constructvariable!(m::Model, t::Symbol; extra_kwargs...)
+    constructvariable!(m, Val{t}; extra_kwargs...)


I'm slightly worried about the overhead in dynamic dispatch here if we're creating 1,000,000 :Cont variables in a loop. Are there any performance regressions?

Yes there might be since the method called cannot be determined only be the type of the arguments

What about creating the Val{:Cont} (etc) types inside the macro when the variable category is known?

Then we might as well modify category directly. The whole point of this method is to support specifying a Symbol in extra but I start to think that this is not a good idea. In PolyJuMP for example since I will create the Poly type I might just as well ask to give Poly instead of :Poly.

Yeah, I don't see a need to support symbols outside of the keyword arguments. Either @variable(m, x, Int) or @variable(m, x, category=:Int).

mlubin · 2017-05-17T20:23:05Z

src/macros.jl

@@ -856,8 +842,7 @@ variable_error(args, str) = error("In @variable($(join(args,","))): ", str)
 # * The `SDP` and `Symmetric` positional arguments in `extra` will not be passed to `constructvariable!`. Instead,
 #    * the `Symmetric` argument will check that the container is symmetric and only allocate one variable for each pair of non-diagonal entries.
 #    * the `SDP` argument will do the same as `Symmetric` but in addition it will specify that the variables created belongs to the SDP cone in the `varCones` field of the model.
-#   Moreover, if a category is passed in `extra` not as a symbol (e.g. `Bin` instead of `:Bin`), it will be transformed to a symbol before being
-#   passed to `constructvariable!`.
+#   Moreover, if a category is passed in `extra`, it will be passed using the `category` keyword to `constructvariable!`.


Could you list the "special" categories that we recognize?

mlubin · 2017-05-17T20:23:53Z

src/macros.jl

-    # if isdefined(Core, :Inference)
-    # however, since there could be symbols in extra, which are only thansformed with Val after, the type could be not inferrable
-    vartype = :( variabletype($m, $(extra...)) )
+    if isdefined(Core, :Inference)


Is this kosher?

blegat · 2017-05-17T20:28:52Z

The variabletype function seems a bit useless now. The only thing that makes it still be useful is the fact that it seems that isdefined(Core, :Inference) may be false (see here). However, I don't understand in which case this might happen

mlubin · 2017-05-17T20:31:51Z

I'd rather have variabletype and avoid using Core.Inference.return_type if we can.

mlubin · 2017-05-17T20:44:13Z

Looks pretty good to me, thanks for addressing all the comments. Could you check for performance regressions by running test/perf/speed2.jl?

blegat · 2017-05-18T09:51:44Z

I had a big 0.5x performance penalty because I was passing lowerbound, ... as keyword arguments to constructvariable!. It is now passed as positional arguments in 2e3f32c.
I now get a performance speedup for CONT5 that I don't understand thanks to the ; extra_kwargs... at the end of constructvariable!. See the details below

➜  perf git:(master) julia5 speed2.jl
PMEDIAN BUILD MIN=0.175563795  MED=0.320741196
PMEDIAN INTRN MIN=0.229743075  MED=0.428653894
CONT5 BUILD   MIN=0.267186116  MED=0.416271085 <- ~25, 40
CONT5 INTRN   MIN=0.116656921  MED=0.241355246
➜  perf git:(master) git co constructvariable\! 
➜  perf git:(constructvariable!) julia5 speed2.jl
PMEDIAN BUILD MIN=0.215696743  MED=0.326776765
PMEDIAN INTRN MIN=0.210215336  MED=0.388464331
CONT5 BUILD   MIN=0.160209637  MED=0.314444853 <- ~15, 30
CONT5 INTRN   MIN=0.118863281  MED=0.254929506
➜  perf git:(constructvariable!) julia5 speed2.jl
PMEDIAN BUILD MIN=0.191695599  MED=0.355724032
PMEDIAN INTRN MIN=0.225102732  MED=0.402638121
CONT5 BUILD   MIN=0.15938412   MED=0.294249865 <- ~15, 30
CONT5 INTRN   MIN=0.116651164  MED=0.240908563
➜  perf git:(constructvariable!) julia5 speed2.jl
PMEDIAN BUILD MIN=0.19490058  MED=0.337533164
PMEDIAN INTRN MIN=0.222318405  MED=0.399466155
CONT5 BUILD   MIN=0.16060928   MED=0.29709146  <- ~15, 30
CONT5 INTRN   MIN=0.117465759  MED=0.238921681
➜  perf git:(constructvariable!) git co master
➜  perf git:(master) julia5 speed2.jl
PMEDIAN BUILD MIN=0.170941645  MED=0.322015502
PMEDIAN INTRN MIN=0.214928813  MED=0.428653469
CONT5 BUILD   MIN=0.266489441  MED=0.392994993 <- ~25, 40
CONT5 INTRN   MIN=0.116713644  MED=0.236784862
➜  perf git:(master) julia5 speed2.jl
PMEDIAN BUILD MIN=0.171263875  MED=0.327381283
PMEDIAN INTRN MIN=0.217781721  MED=0.433130278
CONT5 BUILD   MIN=0.264515939  MED=0.428355359 <- ~25, 40
CONT5 INTRN   MIN=0.117319895  MED=0.238396184
➜  perf git:(master) julia5 speed2.jl
PMEDIAN BUILD MIN=0.166273391  MED=0.316517186
PMEDIAN INTRN MIN=0.221250783  MED=0.427581392
CONT5 BUILD   MIN=0.26114252  MED=0.388637578  <- ~25, 40
CONT5 INTRN   MIN=0.117485551  MED=0.237800865
➜  perf git:(master!) git co constructvariable\!
(remove the for loops for the errors of extra_kwargs)
➜  perf git:(constructvariable!) julia5 speed2.jl          
PMEDIAN BUILD MIN=0.214137355  MED=0.318337662
PMEDIAN INTRN MIN=0.211794955  MED=0.393985586
CONT5 BUILD   MIN=0.15971617   MED=0.312552981 <- ~15, 30
CONT5 INTRN   MIN=0.118461951  MED=0.257029013
(remove the "; extra_kwargs...")
➜  perf git:(constructvariable!) julia5 speed2.jl
PMEDIAN BUILD MIN=0.179196647  MED=0.341086195
PMEDIAN INTRN MIN=0.219295261  MED=0.449476719
CONT5 BUILD   MIN=0.272981907  MED=0.411483141 <- ~25, 40
CONT5 INTRN   MIN=0.119036939  MED=0.248688892
(add everything back)
➜  perf git:(constructvariable!) julia5 speed2.jl
PMEDIAN BUILD MIN=0.21665448  MED=0.32489813
PMEDIAN INTRN MIN=0.214153409  MED=0.393124851
CONT5 BUILD   MIN=0.160253171  MED=0.317724199 <- ~15, 30
CONT5 INTRN   MIN=0.11881246  MED=0.255349231

mlubin · 2017-05-19T21:25:34Z

Does the new change to positional arguments mean that anyone who implements constructvariable! has to take arguments objective::Number, inconstraints::Vector and , coefficients::Vector{Float64}? That seems pretty messy since these arguments are very specific to LinearQuadratic models.

blegat · 2017-05-19T22:08:06Z

No you can implement the second version without these 3 arguments.

mlubin · 2017-05-19T22:10:08Z

Is that explained somewhere in the comments?

blegat · 2017-05-20T14:11:24Z

Now it is :)

mlubin · 2017-05-20T17:05:01Z

Seems good now, but tests are failing.

blegat · 2017-05-21T09:26:43Z

I use the isdependent function to determine if sym depends on the idxvars. If this is not the case, I fall back to the old behavior. Otherwise I do the nested loop to execute sym with values for all of the idxvars. Once it has been executed back I break all the nested loops. By default the type is Any so

@variable x[i=1:0] == i

will be a container of Any.
However

@variable x[i=1:0] == 0

will be a container of Variable since it will detect that sym does not depends on i.

mlubin · 2017-05-21T12:30:02Z

Why would the type of the variable depend on the value of the bounds?

blegat · 2017-05-21T14:18:03Z

For example for polynomials, for p(x), we create one Variable per coefficient of and it creates a Polynomial{JuMP.Variable} but for p(x) >= 0, we create one SDP matrix of variables so it returns a MatPolynomial{JuMP.Variable}.

mlubin · 2017-05-21T15:50:32Z

@blegat, that syntax isn't very consistent with how we use @variable now. Could you use a tag like SDP for this?

blegat · 2017-05-21T15:51:28Z

Sorry I meant p and p >= 0 not p(x) and p(x) >= 0

mlubin · 2017-05-21T18:51:59Z

Still p >= a returning a different type when a = 1 and a = 0 is not consistent with what we do. The types shouldn't depend on data.

blegat · 2017-05-21T19:45:34Z

I could do

@variable(m, p, Poly(X))
@variable(m, p, SOSPoly(x)) equivalent to @variable(m, p >= 0, SOSPoly(x)) -> instead of @variable(m, p >= 0, Poly(X))

What do you prefer then:

Remove getloopedtype and do variabletype(m, extra...) instead of variabletype(m, extra..., lowerbound, upperbound, basename, category, start)
Allow variable type to depend on the type of lowerbound, upperbound, basename, category, start` but not on its value.

mlubin · 2017-05-21T19:47:15Z

The first option is much simpler and preferable unless there's a good reason for the second option.

blegat · 2017-05-21T19:52:28Z

I have implemented the first option in 088c84b :)

mlubin · 2017-05-21T19:55:47Z

To make sure I'm understanding things, you will implement implement variabletype(::Model,::Poly) and variabletype(::Model,::SOSPoly) right?

blegat · 2017-05-21T19:57:40Z

Yes exactly :) (although technically it will be Poly{false} and Poly{true} and SOSPoly will be an alias created by SumOfSquares)

mlubin · 2017-05-21T20:06:17Z

Ok, makes sense. It would be nice if we removed SDP and Symmetric as special cases, but that can be future work.

mlubin · 2017-05-21T20:10:53Z

Final request before merging. Could you add a test that implements a trivial extension and calls, e.g., @variable(m, x, MyType)?

mlubin · 2017-05-22T12:50:02Z

I'll merge this tomorrow if there are no further objections. @joehuchette

joehuchette · 2017-05-23T04:22:25Z

Can we hold off merging for a day or two, until I can review in detail?

mlubin · 2017-05-23T12:41:57Z

@joehuchette, sure

joehuchette

Looks good to me!

joehuchette · 2017-05-24T00:32:50Z

src/macros.jl

 const EMPTYSTRING = ""

 variable_error(args, str) = error("In @variable($(join(args,","))): ", str)

+# @variable(m, expr, extra...; kwargs...)
+# where `extra` is a list of extra positional arguments and `extra_kwargs` is a list of keyword arguments.


extra_kwargs --> kwargs

Add the constructvariable!/variabletype functions

b98c052

This allows extensions to use the @variable macro to create other types of variables

Add comments for @variable, constructvariable! and variabletype

22bc8fe

mlubin reviewed May 17, 2017

View reviewed changes

Remove constructvariable! method for symbol

a759f14

mlubin reviewed May 17, 2017

View reviewed changes

Stop using Core.Inference.return_type and add examples

c908d07

Pass arguments as positional arguments in constructvariable! for speed

2e3f32c

blegat added 2 commits May 20, 2017 16:11

escape extra arguments passed to constructvariable! in @variable

d492dbe

Add missing variabletype

8bdbcbf

blegat added 2 commits May 20, 2017 22:02

Support lb/ub/start depending on index for variabletype

524d2e6

Detect when sym is constant in getloopedtype

bcb4f3d

Make variabletype only depend on extra positional arguments

088c84b

blegat added 4 commits May 21, 2017 23:06

Fix addition of kwargs to constructvariable!

a99710e

Add test for constructvariable! and variabletype

380bcb9

MyType -> MyVariable

2767bab

Fix addkwargs! on v0.6

b5f3e1c

joehuchette approved these changes May 24, 2017

View reviewed changes

joehuchette merged commit 7b5077d into master May 24, 2017

blegat deleted the constructvariable! branch May 25, 2017 09:53

Add the constructvariable!/variabletype functions #1029

Add the constructvariable!/variabletype functions #1029

Conversation

blegat commented May 16, 2017

joehuchette commented May 16, 2017

mlubin commented May 17, 2017

odow commented May 17, 2017

mlubin commented May 17, 2017

codecov bot commented May 17, 2017 • edited Loading

Codecov Report

blegat commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blegat commented May 17, 2017 • edited Loading

mlubin commented May 17, 2017

mlubin commented May 17, 2017

blegat commented May 18, 2017

mlubin commented May 19, 2017

blegat commented May 19, 2017

mlubin commented May 19, 2017

blegat commented May 20, 2017

mlubin commented May 20, 2017

blegat commented May 21, 2017

mlubin commented May 21, 2017

blegat commented May 21, 2017

mlubin commented May 21, 2017

blegat commented May 21, 2017

mlubin commented May 21, 2017

blegat commented May 21, 2017

mlubin commented May 21, 2017

blegat commented May 21, 2017

mlubin commented May 21, 2017

blegat commented May 21, 2017

mlubin commented May 21, 2017

mlubin commented May 21, 2017

mlubin commented May 22, 2017

joehuchette commented May 23, 2017

mlubin commented May 23, 2017

joehuchette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 17, 2017 •

edited

Loading

blegat commented May 17, 2017 •

edited

Loading