Add `rewrite_call_expression`. #2229

dourouc05 · 2020-04-22T22:00:24Z

Excerpt from #2051.

codecov · 2020-04-24T04:23:30Z

Codecov Report

Merging #2229 into master will increase coverage by 0.02%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master    #2229      +/-   ##
==========================================
+ Coverage   91.16%   91.18%   +0.02%     
==========================================
  Files          42       42              
  Lines        4165     4186      +21     
==========================================
+ Hits         3797     3817      +20     
- Misses        368      369       +1

Impacted Files	Coverage Δ
src/macros.jl	`92.65% <96.15%> (+0.12%)`	⬆️

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 a72e7ac...2384699. Read the comment docs.

dourouc05 · 2020-05-04T16:58:22Z

I just rebased this on top of #2228, as it got merged.

@blegat

Add support for reification. Things moved to https://github.com/dourouc05/JuCP.jl Update macros.jl Pass the same type of argument to the different calls to parsefun. Restore two definitions of parse_constraint to avoid ambiguity. LoadError: LoadError: MethodError: parse_constraint(::JuMP.var"#_error#70"{Symbol}, ::Symbol, ::Symbol, ::Expr, ::Symbol, ::Symbol) is ambiguous. Candidates: parse_constraint(_error::Function, sense::Symbol, args...) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:180 parse_constraint(_error::Function, lb, lsign::Symbol, aff, rsign::Symbol, ub) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:205 Implement more closely @blegat's suggestion. Also adapt build_constraint. Also adapt indicator constraints. Also adapt SD constraint. For :call constraint, dispatch on the first argument. Clean up. Make tests pass. Useless extension point. Allow rewriting the rhs of a comparison constraint. Bug fixing (due to MA?). Don't break user code. If the constraint looks like `== f(x)`, whatever f or x, the previous code would always think the function f must be known by JuMP. Reduce diff size. @blegat's comments. Simplify PR wrt jump-dev#2228. Simplify PR wrt jump-dev#2228. Clean PR wrt jump-dev#2229.

@blegat

Add support for reification. Things moved to https://github.com/dourouc05/JuCP.jl Update macros.jl Pass the same type of argument to the different calls to parsefun. Restore two definitions of parse_constraint to avoid ambiguity. LoadError: LoadError: MethodError: parse_constraint(::JuMP.var"#_error#70"{Symbol}, ::Symbol, ::Symbol, ::Expr, ::Symbol, ::Symbol) is ambiguous. Candidates: parse_constraint(_error::Function, sense::Symbol, args...) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:180 parse_constraint(_error::Function, lb, lsign::Symbol, aff, rsign::Symbol, ub) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:205 Implement more closely @blegat's suggestion. Also adapt build_constraint. Also adapt indicator constraints. Also adapt SD constraint. For :call constraint, dispatch on the first argument. Clean up. Make tests pass. Useless extension point. Allow rewriting the rhs of a comparison constraint. Bug fixing (due to MA?). Don't break user code. If the constraint looks like `== f(x)`, whatever f or x, the previous code would always think the function f must be known by JuMP. Reduce diff size. @blegat's comments. Simplify PR wrt jump-dev#2228. Simplify PR wrt jump-dev#2228. Clean PR wrt jump-dev#2229.

@blegat

Add support for reification. Things moved to https://github.com/dourouc05/JuCP.jl Update macros.jl Pass the same type of argument to the different calls to parsefun. Restore two definitions of parse_constraint to avoid ambiguity. LoadError: LoadError: MethodError: parse_constraint(::JuMP.var"#_error#70"{Symbol}, ::Symbol, ::Symbol, ::Expr, ::Symbol, ::Symbol) is ambiguous. Candidates: parse_constraint(_error::Function, sense::Symbol, args...) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:180 parse_constraint(_error::Function, lb, lsign::Symbol, aff, rsign::Symbol, ub) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:205 Implement more closely @blegat's suggestion. Also adapt build_constraint. Also adapt indicator constraints. Also adapt SD constraint. For :call constraint, dispatch on the first argument. Clean up. Make tests pass. Useless extension point. Allow rewriting the rhs of a comparison constraint. Bug fixing (due to MA?). Don't break user code. If the constraint looks like `== f(x)`, whatever f or x, the previous code would always think the function f must be known by JuMP. Reduce diff size. @blegat's comments. Simplify PR wrt jump-dev#2228. Simplify PR wrt jump-dev#2228. Clean PR wrt jump-dev#2229.

@blegat

Add support for reification. Things moved to https://github.com/dourouc05/JuCP.jl Update macros.jl Pass the same type of argument to the different calls to parsefun. Restore two definitions of parse_constraint to avoid ambiguity. LoadError: LoadError: MethodError: parse_constraint(::JuMP.var"#_error#70"{Symbol}, ::Symbol, ::Symbol, ::Expr, ::Symbol, ::Symbol) is ambiguous. Candidates: parse_constraint(_error::Function, sense::Symbol, args...) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:180 parse_constraint(_error::Function, lb, lsign::Symbol, aff, rsign::Symbol, ub) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:205 Implement more closely @blegat's suggestion. Also adapt build_constraint. Also adapt indicator constraints. Also adapt SD constraint. For :call constraint, dispatch on the first argument. Clean up. Make tests pass. Useless extension point. Allow rewriting the rhs of a comparison constraint. Bug fixing (due to MA?). Don't break user code. If the constraint looks like `== f(x)`, whatever f or x, the previous code would always think the function f must be known by JuMP. Reduce diff size. @blegat's comments. Simplify PR wrt jump-dev#2228. Simplify PR wrt jump-dev#2228. Clean PR wrt jump-dev#2229.

@blegat

Add support for reification. Things moved to https://github.com/dourouc05/JuCP.jl Update macros.jl Pass the same type of argument to the different calls to parsefun. Restore two definitions of parse_constraint to avoid ambiguity. LoadError: LoadError: MethodError: parse_constraint(::JuMP.var"#_error#70"{Symbol}, ::Symbol, ::Symbol, ::Expr, ::Symbol, ::Symbol) is ambiguous. Candidates: parse_constraint(_error::Function, sense::Symbol, args...) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:180 parse_constraint(_error::Function, lb, lsign::Symbol, aff, rsign::Symbol, ub) in JuMP at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:205 Implement more closely @blegat's suggestion. Also adapt build_constraint. Also adapt indicator constraints. Also adapt SD constraint. For :call constraint, dispatch on the first argument. Clean up. Make tests pass. Useless extension point. Allow rewriting the rhs of a comparison constraint. Bug fixing (due to MA?). Don't break user code. If the constraint looks like `== f(x)`, whatever f or x, the previous code would always think the function f must be known by JuMP. Reduce diff size. @blegat's comments. Simplify PR wrt jump-dev#2228. Simplify PR wrt jump-dev#2228. Clean PR wrt jump-dev#2229.

blegat · 2020-05-05T13:22:24Z

src/macros.jl

+    end
+    if isexpr(lhs, :call) && expression_to_rewrite(Val(lhs.args[1]), lhs.args[2:end]...)
+        parse_code_lhs, build_code_lhs, new_lhs = rewrite_call_expression(_error, Val(lhs.args[1]), lhs.args[2:end]...)
+    end


Maybe do

parse_code_hhs, build_code_rhs, new_rhs = parse_expression(rhs)

to factor out the copy-past with lhs in parse_expression.

blegat · 2020-05-05T13:25:38Z

src/macros.jl

+# - code for parsing (which must be called first), if needed; otherwise, :().
+# - code for building the required constraints, if needed; otherwise, :().
+# - the symbol of the variable that replaces the expression (<: VariableRef).
+function rewrite_call_expression(_error::Function, head::Val{F}, args...) where F


If we remove expression_to_rewrite, we can give a default for this function of :(), :(), rhs instead of an error and everything would work with only one function to implement instead of two. Is that correct or am I missing something ?

If you write a constraint like @constraint(m, sum(1 for _ in 1:10) == sum(1 for _ in 1:10)), then you have a :call expression on both sides, but it should not be rewritten (or any function that should not be caught by JuMP).

Yes but if you replace the current fallback throwing an error by return :(), :(), Expr(...), it will work for these cases too

If I rebuild the expression like this:

function rewrite_call_expression(_error::Function, head::Val{F}, args...) where F return :(), :(), Expr(:call, [F, args...]) end

then it's MutableArithmetics that complains about the construct:

MutableArithmetics.Zero (Issue #2187): Error During Test at C:\Users\Thibaut\.julia\dev\JuMP\test\macros.jl:50 Got exception outside of a @test MethodError: objects of type Array{Any,1} are not callable Use square brackets [] for indexing an Array. Stacktrace: [1] top-level scope at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:218 [2] top-level scope at C:\Users\Thibaut\.julia\dev\JuMP\test\macros.jl:52 [3] top-level scope at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\Test\src\Test.jl:1113 [4] top-level scope at C:\Users\Thibaut\.julia\dev\JuMP\test\macros.jl:51 [5] include(::String) at .\client.jl:439 [6] top-level scope at C:\Users\Thibaut\.julia\dev\JuMP\test\runtests.jl:23 [7] include(::String) at .\client.jl:439 [8] top-level scope at none:6 [9] eval(::Module, ::Any) at .\boot.jl:331 [10] exec_options(::Base.JLOptions) at .\client.jl:264 [11] _start() at .\client.jl:484

By the way, what would be your preferred way of implementing rewriting operations like needed in dourouc05@e36c196#diff-6f46b954dc01974c3fb3f315681c3059R217-R247? Right now, the first error happens in MutableArithmetics, but I guess this package has nothing to do with this kind of rewritings, so maybe having a _rewrite_expr() function in JuMP would make sense?

The branch https://github.com/dourouc05/JuMP.jl/tree/dourouc05-rewrite-complex implements the idea of a rewriting function. It probably has some cost at runtime when building the model (each expression is fully traversed by this step, and it's mostly unavoidable to keep the same level of functionality), but it allows a lot of flexibility. In particular, this would allow parsing things like a && b without trouble. It's still orthogonal to #2228, which allows supporting new operators (like :=): here, it's really for the usual <=, >=, == constraints with funny things in either side.

Also, it's easy to disable: the main calls to _rewrite_expr could be disabled if the user requires it, for instance.

Shall I update this PR to include the new commits?

However, there is still one thing to discuss: how to remove a constraint like 1 + f(x) <= y from JuMP in case f adds a new variable and one constraint to do its job (like z == x^2, say)? When dealing with f(x), build_constraint will have to build z == x^2; when dealing with the main constraint, it will only look like 1 + z <= y, and build_constraint will have no idea that a new constraint was added.

My idea would be to return a list of constraint objects along with parse_code and build_code, which would be passed on to the final build_constraint. ScalarConstraint and VectorConstraint would then also have a vector of MOI constraints that must be deleted at the same time.

About the constraint to remove, it seems to me that you need to create a custom set and a bridge for this set. In the bridge you can create new variables and constraints. When the constraint is deleted, the bridge is deleted too and in the deletion of the bridge, you can delete the added variables and constraints.

Traversing the expressions has no runtime cost, the macros are evaluated during compilation.

What bridge would be used for 1 + f(x) <= y? What I'm pointing is that this code is used before bridges: a bridge can be used to map a constraint z == x^2 to something the solver understands, but not for a constraint 1 + f(x) <= y that it does not understand (there is no way of having a scalar like 1 + f(x) right now in MOI). This code allows to add new constraints to get from a nice user-facing syntax to MOI-level constraints, which can then be bridged depending on the solver abilities.

Take 1 + count(x .== 4) <= y, for instance. This code allows to create two MOI constraints: z = count(x .== 4) and 1 + z <= y, i.e. MOI.add_constraint(model, [z, x], CP.Count(4)) and MOI.add_constraint(model, z - y, MOI.LessThan(1.0)). The first constraint can be bridged to provide a MILP formulation, for instance. However, MOI does not accept something like MOI.add_constraint(model, 1 + count(x .== 4), MOI.LessThan(4.0)). As I understand it, rewriting/lowering the constraint to MOI level (a "thin" abstraction on top of solvers) is JuMP's responsibility, as what's done for quadratic constraints (rewritten as SOCP when possible: https://www.juliaopt.org/JuMP.jl/stable/constraints/#Quadratic-constraints-1).

It looks like compile-time performance is still a goal, as per the benchmarks of https://mlubin.github.io/pdf/jump-sirev.pdf. Or things did evolve since then? But still, I don't think performance issues should be discussed now, at least before any serious benchmarks occur and shows this part to be a significant bottleneck.

dourouc05 · 2020-05-07T13:47:34Z

I cannot yet remove the strange code for MutableArithmetics (https://github.com/JuliaOpt/JuMP.jl/pull/2229/files#diff-0033f9de61e25a98b730785191eb2c68R190-R208) due to this error:

MutableArithmetics.Zero (Issue #2187): Error During Test at C:\Users\Thibaut\.julia\dev\JuMP\test\macros.jl:50
  Got exception outside of a @test
  ArgumentError: reducing over an empty collection is not allowed
  Stacktrace:
   [1] _empty_reduce_error() at .\reduce.jl:295
   [2] mapreduce_empty(::Function, ::Base.BottomRF{typeof(Base.add_sum)}, ::Type{T} where T) at .\reduce.jl:334
   [3] reduce_empty(::Base.MappingRF{var"#3#5",Base.BottomRF{typeof(Base.add_sum)}}, ::Type{T} where T) at .\reduce.jl:321
   [4] reduce_empty_iter at .\reduce.jl:347 [inlined]
   [5] reduce_empty_iter at .\reduce.jl:346 [inlined]
   [6] foldl_impl at .\reduce.jl:46 [inlined]
   [7] mapfoldl_impl at .\reduce.jl:41 [inlined]
   [8] #mapfoldl#189 at .\reduce.jl:157 [inlined]
   [9] mapfoldl at .\reduce.jl:157 [inlined]
   [10] #mapreduce#193 at .\reduce.jl:283 [inlined]
   [11] mapreduce at .\reduce.jl:283 [inlined]
   [12] sum at .\reduce.jl:486 [inlined]
   [13] sum(::Base.Generator{UnitRange{Int64},var"#3#5"}) at .\reduce.jl:503
   [14] top-level scope at C:\Users\Thibaut\.julia\dev\JuMP\src\macros.jl:192
   [15] top-level scope at C:\Users\Thibaut\.julia\dev\JuMP\test\macros.jl:52
   [16] top-level scope at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\Test\src\Test.jl:1113
   [17] top-level scope at C:\Users\Thibaut\.julia\dev\JuMP\test\macros.jl:51
   [18] include(::String) at .\client.jl:439
   [19] top-level scope at C:\Users\Thibaut\.julia\dev\JuMP\test\runtests.jl:23
   [20] include(::String) at .\client.jl:439
   [21] top-level scope at none:6
   [22] eval(::Module, ::Any) at .\boot.jl:331
   [23] exec_options(::Base.JLOptions) at .\client.jl:264
   [24] _start() at .\client.jl:484

However, I don't have this one with master...dourouc05:dourouc05-rewrite-complex…

dourouc05 mentioned this pull request Apr 24, 2020

Add extension points for more generic @constraint syntax #2051

Merged

dourouc05 added a commit to dourouc05/JuMP.jl that referenced this pull request Apr 24, 2020

Clean macro wrt jump-dev#2229.

57c9d76

dourouc05 added a commit to dourouc05/JuMP.jl that referenced this pull request Apr 24, 2020

Clean PR wrt jump-dev#2229.

9c5d812

dourouc05 added a commit to dourouc05/JuMP.jl that referenced this pull request Apr 24, 2020

Clean PR wrt jump-dev#2229.

e481b15

dourouc05 added a commit to dourouc05/JuMP.jl that referenced this pull request Apr 24, 2020

Clean PR wrt jump-dev#2229.

dc77f32

dourouc05 mentioned this pull request Apr 24, 2020

Improve support for constraint programming #2227

Closed

35 tasks

dourouc05 added 6 commits May 4, 2020 18:55

Add rewrite_call_expression.

d857771

Problem with copy-paste.

565f60a

Allow expression rewriting on both sides. Add tests.

1131443

Make the tests pass.

e8726d5

Implement the case where both sides are rewritten. Add a little doc.

00ad3cb

Don't forget to actually test something…

2145762

dourouc05 force-pushed the dourouc05-rewrite branch from 1acccf1 to 2145762 Compare May 4, 2020 16:58

Bad rebase.

491af4d

blegat reviewed May 5, 2020

View reviewed changes

dourouc05 added 4 commits May 7, 2020 01:42

Always enable.

a761ce0

Change that got merged into the next commit.

8d4fae6

Simplify code a bit.

395be9e

Update macros.jl

fdef385

Try to simplify (does not work).

2384699

dourouc05 mentioned this pull request May 12, 2020

Add rewrite_call_expression through _rewrite_expr. #2241

Closed

dourouc05 closed this Nov 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `rewrite_call_expression`. #2229

Add `rewrite_call_expression`. #2229

dourouc05 commented Apr 22, 2020

codecov bot commented Apr 24, 2020 •

edited

dourouc05 commented May 4, 2020

blegat May 5, 2020

blegat May 5, 2020

dourouc05 May 5, 2020

blegat May 6, 2020

dourouc05 May 6, 2020

dourouc05 May 6, 2020

dourouc05 May 7, 2020

blegat May 7, 2020

blegat May 7, 2020

dourouc05 May 7, 2020

dourouc05 commented May 7, 2020

Add rewrite_call_expression. #2229

Add rewrite_call_expression. #2229

Conversation

dourouc05 commented Apr 22, 2020

codecov bot commented Apr 24, 2020 • edited

Codecov Report

dourouc05 commented May 4, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dourouc05 commented May 7, 2020

Add `rewrite_call_expression`. #2229

Add `rewrite_call_expression`. #2229

codecov bot commented Apr 24, 2020 •

edited