From 367daee57e4c423c54a7f9d9b1900c9da6490631 Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 7 Oct 2021 14:39:24 +1300 Subject: [PATCH 01/10] WIP: refactor the parse_constraint methods --- docs/src/reference/extensions.md | 2 +- src/complement.jl | 2 +- src/indicator.jl | 4 +- src/macros.jl | 357 ++++++++++++++++--------------- test/constraint.jl | 5 +- test/macros.jl | 7 +- 6 files changed, 194 insertions(+), 183 deletions(-) diff --git a/docs/src/reference/extensions.md b/docs/src/reference/extensions.md index eeea64899eb..3fb0b8defae 100644 --- a/docs/src/reference/extensions.md +++ b/docs/src/reference/extensions.md @@ -23,7 +23,7 @@ build_variable ```@docs build_constraint add_constraint -sense_to_set +operator_to_set AbstractShape shape reshape_vector diff --git a/src/complement.jl b/src/complement.jl index 17e0d7c181f..0f8c782c8c3 100644 --- a/src/complement.jl +++ b/src/complement.jl @@ -63,7 +63,7 @@ function _build_complements_constraint( return errorf("second term must be a variable.") end -function parse_one_operator_constraint( +function parse_constraint_call( errorf::Function, ::Bool, ::Union{Val{:complements},Val{:⟂}}, diff --git a/src/indicator.jl b/src/indicator.jl index e40e4c0bd26..4063916b4c9 100644 --- a/src/indicator.jl +++ b/src/indicator.jl @@ -30,7 +30,7 @@ function _indicator_variable_set(_error::Function, expr::Expr) return expr, MOI.Indicator{MOI.ACTIVATE_ON_ONE} end end -function parse_one_operator_constraint( +function parse_constraint_call( _error::Function, vectorized::Bool, ::Union{Val{:(=>)},Val{:⇒}}, @@ -45,7 +45,7 @@ function parse_one_operator_constraint( end rhs_con = rhs.args[1] rhs_vectorized, rhs_parsecode, rhs_buildcall = - parse_constraint_expr(_error, rhs_con) + parse_constraint(_error, rhs_con) if vectorized != rhs_vectorized _error("Inconsistent use of `.` in symbols to indicate vectorization.") end diff --git a/src/macros.jl b/src/macros.jl index 230e637af0e..03cd256c3fe 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -159,11 +159,8 @@ function _check_vectorized(sense::Symbol) end end -# two-argument build_constraint is used for one-sided constraints. -# Right-hand side is zero. - """ - sense_to_set(_error::Function, ::Val{sense_symbol}) + operator_to_set(_error::Function, ::Val{sense_symbol}) Converts a sense symbol to a set `set` such that `@constraint(model, func sense_symbol 0)` is equivalent to @@ -172,7 +169,7 @@ Converts a sense symbol to a set `set` such that ## Example Once a custom set is defined you can directly create a JuMP constraint with it: -```jldoctest sense_to_set; setup = :(using JuMP) +```jldoctest operator_to_set; setup = :(using JuMP) julia> struct CustomSet{T} <: MOI.AbstractScalarSet value::T end @@ -190,8 +187,8 @@ x ∈ CustomSet{Float64}(1.0) However, there might be an appropriate sign that could be used in order to provide a more convenient syntax: -```jldoctest sense_to_set -julia> JuMP.sense_to_set(::Function, ::Val{:⊰}) = CustomSet(0.0) +```jldoctest operator_to_set +julia> JuMP.operator_to_set(::Function, ::Val{:⊰}) = CustomSet(0.0) julia> MOIU.shift_constant(set::CustomSet, value) = CustomSet(set.value + value) @@ -202,16 +199,17 @@ Note that the whole function is first moved to the right-hand side, then the sign is transformed into a set with zero constant and finally the constant is moved to the set with `MOIU.shift_constant`. """ -function sense_to_set end +function operator_to_set end -function sense_to_set(_error::Function, ::Union{Val{:(<=)},Val{:(≤)}}) - return MOI.LessThan(0.0) -end -function sense_to_set(_error::Function, ::Union{Val{:(>=)},Val{:(≥)}}) +operator_to_set(::Function, ::Union{Val{:(<=)},Val{:(≤)}}) = MOI.LessThan(0.0) + +function operator_to_set(::Function, ::Union{Val{:(>=)},Val{:(≥)}}) return MOI.GreaterThan(0.0) end -sense_to_set(_error::Function, ::Val{:(==)}) = MOI.EqualTo(0.0) -function sense_to_set(_error::Function, ::Val{S}) where {S} + +operator_to_set(::Function, ::Val{:(==)}) = MOI.EqualTo(0.0) + +function operator_to_set(_error::Function, ::Val{S}) where {S} return _error("Unrecognized sense $S") end @@ -259,27 +257,6 @@ array, and wait for complaints. _desparsify(x::AbstractSparseArray) = collect(x) _desparsify(x) = x -function _build_call(_error::Function, vectorized::Bool, func, set) - return if vectorized - :(build_constraint.($_error, _desparsify($func), Ref($(esc(set))))) - else - :(build_constraint($_error, $func, $(esc(set)))) - end -end -function parse_one_operator_constraint( - _error::Function, - vectorized::Bool, - ::Union{Val{:in},Val{:∈}}, - func, - set, -) - variable, parse_code = _MA.rewrite(func) - return parse_code, _build_call(_error, vectorized, variable, set) -end -function parse_one_operator_constraint(_error::Function, args...) - return _unknown_constraint_expr(_error) -end - _functionize(v::VariableRef) = convert(AffExpr, v) _functionize(v::AbstractArray{VariableRef}) = _functionize.(v) @@ -292,57 +269,101 @@ end _functionize(x) = x _functionize(::MutableArithmetics.Zero) = 0.0 -function parse_one_operator_constraint( - _error::Function, - vectorized::Bool, - sense::Val, - lhs, - rhs, -) - # Simple comparison - move everything to the LHS. - # - # `_functionize` deals with the pathological case where the `lhs` is a `VariableRef` - # and the `rhs` is a summation with no terms. `_build_call` should be passed a - # `GenericAffExpr` or a `GenericQuadExpr`, and not a `VariableRef` as would be the case - # without `_functionize`. - if vectorized - func = :($lhs .- $rhs) - else - func = :($lhs - $rhs) - end - set = sense_to_set(_error, sense) - variable, parse_code = _MA.rewrite(func) - return parse_code, - _build_call(_error, vectorized, :(_functionize($variable)), set) -end +""" + parse_constraint(_error::Function, expr::Expr) + +The entry-point for all constraint-related parsing. + +## Arguments -function parse_constraint_expr(_error::Function, expr::Expr) + * The `_error` function is passed everywhere to provide better error messages + * `expr` comes from the `@constraint` macro. There are two possibilities: + * `@constraint(model, expr)` + * `@constraint(model, name[args], expr)` + In both cases, `expr` is the main component of the constraint. + +JuMP currently supports the following `expr` objects: + * `lhs <= rhs` + * `lhs == rhs` + * `lhs >= rhs` + * `l <= body <= u` + * `u >= body >= l` + * `lhs ⟂ rhs` + * `lhs in rhs` + * `lhs ∈ rhs` + * `z => {constraint}` + * `!z => {constraint}` +as well as all broadcasted variants. + +## See also + +The aim of `parse_constraint` is to make this extensible by JuMP extensions. +Thus, `parse_constraint` forwards to [`parse_constraint_head`](@ref) to dispatch +on `expr.head` +""" +function parse_constraint(_error::Function, expr::Expr) return parse_constraint_head(_error, Val(expr.head), expr.args...) end -function parse_constraint_head(_error::Function, ::Val{:call}, args...) - return parse_constraint(_error, args...) + +""" + parse_constraint_head(_error::Function, ::Val{head}, args...) + +Implement this method to intercept the parsing of an expression with head +`head`. + +JuMP currently implements: + + * `::Val{:call}`, which forwards calls to [`parse_constraint_call`](@ref) + * `::Val{:comparison}`, which handles the special case of `l <= body <= u`. + +!!! warning + Think carefully before doing this! By implementing this method, your package + will own the sytax. Do not implement a method for syntax that JuMP already + supports. +""" +function parse_constraint_head(_error::Function, ::Val{T}, args...) where {T} + return _error( + "Unsupported constraint expression: we don't know how to parse " * + "constraints containing expressions of type $T.\n\nIf you are " * + "writing a JuMP extension, implement " * + "`parse_constraint_head(::Function, ::Val{$T}, args...)", + ) end -function parse_constraint(_error::Function, sense::Symbol, lhs, rhs) - (sense, vectorized) = _check_vectorized(sense) - return vectorized, - parse_one_operator_constraint(_error, vectorized, Val(sense), lhs, rhs)... +function parse_constraint_head(_error::Function, ::Val{:call}, args...) + return parse_constraint_call(_error, args...) end -function parse_ternary_constraint( +function parse_constraint_head( _error::Function, - vectorized::Bool, + ::Val{:comparison}, lb, - ::Union{Val{:(<=)},Val{:(≤)}}, + lsign::Symbol, aff, - rsign::Union{Val{:(<=)},Val{:(≤)}}, + rsign::Symbol, ub, ) - newaff, parseaff = _MA.rewrite(aff) - newlb, parselb = _MA.rewrite(lb) - newub, parseub = _MA.rewrite(ub) - if vectorized - buildcall = :( + lsign, lvectorized = _check_vectorized(lsign) + rsign, rvectorized = _check_vectorized(rsign) + if lvectorized != rvectorized + _error("Signs are inconsistently vectorized") + end + if lsign in (:(<=), :≤) && rsign in (:(<=), :≤) + # Nothing. What we expect. + elseif lsign in (:(>=), :≥) && rsign in (:(>=), :≥) + # Flip lb and ub + lb, ub = ub, lb + else + _error( + "Only two-sided rows of the form `lb <= expr <= ub` or " * + "`ub >= expr >= lb` are supported.", + ) + end + newaff, parse_aff = _MA.rewrite(aff) + newlb, parse_lb = _MA.rewrite(lb) + newub, parse_ub = _MA.rewrite(ub) + build_call = if lvectorized + :( build_constraint.( $_error, _desparsify($newaff), @@ -351,111 +372,115 @@ function parse_ternary_constraint( ) ) else - buildcall = :(build_constraint($_error, $newaff, $newlb, $newub)) + :(build_constraint($_error, $newaff, $newlb, $newub)) + end + parse_code = quote + $parse_aff + $parse_lb + $parse_ub end - return parseaff, parselb, parseub, buildcall + return lvectorized, parse_code, build_call end -function parse_ternary_constraint( - _error::Function, - vectorized::Bool, - ub, - ::Union{Val{:(>=)},Val{:(≥)}}, - aff, - rsign::Union{Val{:(>=)},Val{:(≥)}}, - lb, -) - return parse_ternary_constraint( - _error, - vectorized, - lb, - Val(:(<=)), - aff, - Val(:(<=)), - ub, +""" + parse_constraint_call( + _error::Function, + vectorized::Bool, + ::Val{op}, + args..., ) -end -function parse_ternary_constraint(_error::Function, args...) +Parse constraints of the form: +```julia +@constraint(model, op(args...)) +``` +If `vectorized`, the operation is broadcasted, so the constraint is of the form +```julia +@constraint(model, op.(args...)) +``` + +This function returns a `(parse_code, build_call)` tuple. +""" +function parse_constraint_call( + _error::Function, + ::Bool, + ::Val{T}, + args..., +) where {T} return _error( - "Only two-sided rows of the form lb <= expr <= ub or ub >= expr >= lb are supported.", + "Unsupported constraint expression: we don't know how to parse " * + "constraints containing the operator $T.\n\nIf you are writing a " * + "JuMP extension, implement " * + "`parse_constraint_call(::Function, ::Bool, ::Val{$T}, args...)", ) end -function parse_constraint_head( - _error::Function, - ::Val{:comparison}, - lb, - lsign::Symbol, - aff, - rsign::Symbol, - ub, -) - return parse_constraint(_error, lb, lsign, aff, rsign, ub) +function parse_constraint_call(_error::Function, operator::Symbol, args...) + operator, vectorized = _check_vectorized(operator) + parse_code, build_call = + parse_constraint_call(_error, vectorized, Val(operator), args...) + return vectorized, parse_code, build_call end -function parse_constraint( +# `@constraint(model, func in set)` +# `@constraint(model, func ∈ set)` +function parse_constraint_call( _error::Function, - lb, - lsign::Symbol, - aff, - rsign::Symbol, - ub, + vectorized::Bool, + ::Union{Val{:in},Val{:∈}}, + func, + set, ) - (lsign, lvectorized) = _check_vectorized(lsign) - (rsign, rvectorized) = _check_vectorized(rsign) - ((vectorized = lvectorized) == rvectorized) || - _error("Signs are inconsistently vectorized") - parseaff, parselb, parseub, buildcall = parse_ternary_constraint( - _error, - vectorized, - lb, - Val(lsign), - aff, - Val(rsign), - ub, - ) - parsecode = quote - $parseaff - $parselb - $parseub + f, parse_code = _MA.rewrite(func) + build_call = if vectorized + :(build_constraint.($_error, _desparsify($f), Ref($(esc(set))))) + else + :(build_constraint($_error, $f, $(esc(set)))) end - return vectorized, parsecode, buildcall + return parse_code, build_call end -function _unknown_constraint_expr(_error::Function) - # Unknown - return _error( - "Constraints must be in one of the following forms:\n" * - " expr1 <= expr2\n" * - " expr1 >= expr2\n" * - " expr1 == expr2\n" * - " lb <= expr <= ub", +""" + parse_constraint_call( + _error::Function, + vectorized::Bool, + operator::Val, + lhs, + rhs, ) -end -function parse_constraint_head(_error::Function, ::Val, args...) - return _unknown_constraint_expr(_error) -end -function parse_constraint(_error::Function, args...) - # Define this as the last fallback: either this is a function call that may - # be overridden by extensions, or a syntax that is not recognized. - # Multiple dispatch does not work here, due to ambiguity with: - # parse_constraint(_error::Function, lb, lsign::Symbol, aff, rsign::Symbol, ub) - if args[1] isa Symbol - (sense, vectorized) = _check_vectorized(args[1]) - vectorized, - parse_one_operator_constraint( - _error, - vectorized, - Val(sense), - args[2:end]..., - )... +Fallback handler for binary operators. These might be infix operators like +`@constraint(model, lhs op rhs)`, or normal operators like +`@constraint(model, op(lhs, rhs))`. + +In both cases, we rewrite as `lhs - rhs in operator_to_set(_error, op)`. +See [`operator_to_set`](@ref) for details. +""" +function parse_constraint_call( + _error::Function, + vectorized::Bool, + operator::Val, + lhs, + rhs, +) + func = vectorized ? :($lhs .- $rhs) : :($lhs - $rhs) + set = operator_to_set(_error, operator) + f, parse_code = _MA.rewrite(func) + # `_functionize` deals with the pathological case where the `lhs` is a + # `VariableRef` and the `rhs` is a summation with no terms. + f = :(_functionize($f)) + build_call = if vectorized + :(build_constraint.($_error, _desparsify($f), Ref($(esc(set))))) else - _unknown_constraint_expr(_error) + :(build_constraint($_error, $f, $(esc(set)))) end + return parse_code, build_call end +### +### Build constraints using actual data. +### + # Generic fallback. function build_constraint(_error::Function, func, set, args...; kwargs...) arg_str = join(args, ", ") @@ -834,12 +859,7 @@ enable this syntax by defining extensions of user syntax: `@constraint(model, ref[...], expr, my_arg, kw_args...)`. """ macro constraint(args...) - return _constraint_macro( - args, - :constraint, - parse_constraint_expr, - __source__, - ) + return _constraint_macro(args, :constraint, parse_constraint, __source__) end function parse_SD_constraint_expr(_error::Function, expr::Expr) @@ -865,13 +885,8 @@ function parse_SD_constraint(_error::Function, sense::Symbol, lhs, rhs) aff = :($succ - $prec) end vectorized = false - parsecode, buildcall = parse_one_operator_constraint( - _error, - false, - Val(:in), - aff, - :(JuMP.PSDCone()), - ) + parsecode, buildcall = + parse_constraint_call(_error, false, Val(:in), aff, :(JuMP.PSDCone())) return vectorized, parsecode, buildcall end @@ -997,7 +1012,7 @@ macro build_constraint(constraint_expr) end is_vectorized, parse_code, build_call = - parse_constraint_expr(_error, constraint_expr) + parse_constraint(_error, constraint_expr) result_variable = gensym() code = quote $parse_code diff --git a/test/constraint.jl b/test/constraint.jl index 72d5b5c22c4..d6284b8abca 100644 --- a/test/constraint.jl +++ b/test/constraint.jl @@ -447,8 +447,8 @@ function test_SDP_constraint(ModelType, VariableRefType) err = ErrorException( "In `@SDconstraint(m, [x 1; 1 -y] ⪰ [1 x; x -2], unknown_kw = 1)`:" * " Unrecognized constraint building format. Tried to invoke " * - "`build_constraint(error, $(aff_str)[x - " * - "1 -x + 1; -x + 1 -y + 2], PSDCone(); unknown_kw = 1)`, but no " * + "`build_constraint(error, JuMP.$(aff_str)[x - " * + "1 -x + 1; -x + 1 -y + 2], JuMP.PSDCone(); unknown_kw = 1)`, but no " * "such method exists. This is due to specifying an unrecognized " * "function, constraint set, and/or extra positional/keyword " * "arguments.\n\nIf you're trying to create a JuMP extension, you " * @@ -701,6 +701,7 @@ function test_Model_all_constraints_vector(::Any, ::Any) "`$(GenericAffExpr{Float64})` is not a concrete type. Did you miss a " * "type parameter?", ) + err = ErrorException(replace(err.msg, "JuMP." => "")) @test_throws err try num_constraints( model, diff --git a/test/macros.jl b/test/macros.jl index 9f9c775c3c7..99a043f3ab1 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -111,12 +111,7 @@ function JuMP.build_constraint(_error::Function, func, ::CustomType) return JuMP.build_constraint(_error, func, CustomSet()) end -function JuMP.parse_one_operator_constraint( - _error::Function, - ::Bool, - ::Val{:f}, - x, -) +function JuMP.parse_constraint_call(_error::Function, ::Bool, ::Val{:f}, x) return :(), :(build_constraint($_error, $(esc(x)), $(esc(CustomType())))) end From 0b593592849b29da1d869d7df28b20971e6af5c8 Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 7 Oct 2021 15:29:22 +1300 Subject: [PATCH 02/10] Add documentation --- docs/src/developers/extensions.md | 125 +++++++++++++++++++++++------- docs/src/reference/extensions.md | 4 +- src/indicator.jl | 2 +- src/macros.jl | 14 ++-- test/constraint.jl | 5 +- 5 files changed, 110 insertions(+), 40 deletions(-) diff --git a/docs/src/developers/extensions.md b/docs/src/developers/extensions.md index b8550ceb50f..10dadc311d9 100644 --- a/docs/src/developers/extensions.md +++ b/docs/src/developers/extensions.md @@ -142,39 +142,102 @@ x[2]_b ## Extend [`@constraint`](@ref) -The [`@constraint`](@ref) macro always calls the same three functions: -* `parse_constraint`: is called at parsing time, it parses the constraint - expression and returns a [`build_constraint`](@ref) call expression; -* [`build_constraint`](@ref): given the functions and sets involved in the - constraints, it returns a `AbstractConstraint`; -* [`add_constraint`](@ref): given the model, the `AbstractConstraint` - constructed in [`build_constraint`](@ref) and the constraint name, it stores - them in the model and returns a `ConstraintRef`. +The [`@constraint`](@ref) macro has three steps that can be intercepted and +extended: parse time, build time, and add time. -Adding methods to these functions is the recommended way to extend the -[`@constraint`](@ref) macro. +### Parse -### Adding `parse_constraint` methods +To extend the [`@constraint`](@ref) macro at parse time, implement one of the +following methods: -Work in progress. -### Adding `build_constraint` methods + * [`parse_constraint_head`](@ref) + * [`parse_constraint_call`](@ref) + +!!! warning + Extending the constraint macro at parse time is an advanced operation and + has the potential to interfere with existing JuMP syntax. Please discuss + with the [developer chatroom](https://gitter.im/JuliaOpt/jump-dev) before + publishing any code that implements these methods. -There are typically two choices when creating a [`build_constraint`](@ref) -method, either return an `AbstractConstraint` already supported by the -model, i.e. `ScalarConstraint` or `VectorConstraint`, or a custom -`AbstractConstraint` with a corresponding [`add_constraint`](@ref) method (see -[Adding `add_constraint` methods](@ref)). +[`parse_constraint_head`](@ref) should be implemented to intercept an expression +based on the `.head` field. For example: +```jldoctest +julia> using JuMP -### Adding `add_constraint` methods +julia> const MutableArithmetics = JuMP._MA; -Work in progress. +julia> model = Model(); @variable(model, x); -### Adding an extra positional argument +julia> function JuMP.parse_constraint_head( + _error::Function, + ::Val{:(:=)}, + lhs, + rhs, + ) + println("Rewriting := as ==") + new_lhs, parse_code = MutableArithmetics.rewrite(lhs) + build_code = :( + build_constraint($(_error), $(new_lhs), MOI.EqualTo($(rhs))) + ) + return false, parse_code, build_code + end -We can also extend `@constraint` to handle additional positional arguments that -effectively "tag" a particular constraint type and/or pass along additional -information that we may want. For example, we can make a `MyConstrType` that -modifies affine equalities: +julia> @constraint(model, x + x := 1.0) +Rewriting := as == +2 x = 1.0 +``` + +[`parse_constraint_call`](@ref) should be implemented to intercept an expression +of the form `Expr(:call, op, args...)`. For example: +```jldoctest +julia> using JuMP + +julia> const MutableArithmetics = JuMP._MA; + +julia> model = Model(); @variable(model, x); + +julia> function JuMP.parse_constraint_call( + _error::Function, + is_vectorized::Bool, + ::Val{:my_equal_to}, + lhs, + rhs, + ) + println("Rewriting my_equal_to to ==") + new_lhs, parse_code = MutableArithmetics.rewrite(lhs) + build_code = if is_vectorized + :(build_constraint($(_error), $(new_lhs), MOI.EqualTo($(rhs))) + ) + else + :(build_constraint.($(_error), $(new_lhs), MOI.EqualTo($(rhs)))) + end + return parse_code, build_code + end + +julia> @constraint(model, my_equal_to(x + x, 1.0)) +Rewriting my_equal_to to == +2 x = 1.0 +``` + +### Build + +To extend the [`@constraint`](@ref) macro at build time, implement a new +[`build_constraint`](@ref) method. + +This may mean implementing a method for a specific function or set created at +parse time, or it may mean implementing a method which handles additional +positional arguments. + +[`build_constraint`](@ref) must return an [`AbstractConstraint`](@ref), which +can either be an [`AbstractConstraint`](@ref) already supported by JuMP, e.g., `ScalarConstraint` or `VectorConstraint`, or a custom +[`AbstractConstraint`](@ref) with a corresponding [`add_constraint`](@ref) +method (see [Add](@ref extension_add_constraint)). + +!!! tip + The easiest way to extend [`@constraint`](@ref) is via an additional + positional argument to [`build_constraint`](@ref). + +Here is an example of adding extra arguments to [`build_constraint`](@ref): ```jldoctest julia> model = Model(); @variable(model, x); @@ -194,9 +257,15 @@ julia> function JuMP.build_constraint( julia> @constraint(model, my_con, x == 0, MyConstrType, d = 2) my_con : x ≤ 2.0 ``` -Note that only a single positional argument can be given to a particular -constraint. Extensions that seek to pass multiple arguments (e.g., `Foo` and -`Bar`) should combine them into one argument type (e.g., `FooBar`). + +!!! note + Only a single positional argument can be given to a particular constraint. + Extensions that seek to pass multiple arguments (e.g., `Foo` and `Bar`) + should combine them into one argument type (e.g., `FooBar`). + +### [Add](@id extension_add_constraint) + +Work in progress. ### Shapes diff --git a/docs/src/reference/extensions.md b/docs/src/reference/extensions.md index 3fb0b8defae..4d0f0f5771f 100644 --- a/docs/src/reference/extensions.md +++ b/docs/src/reference/extensions.md @@ -23,7 +23,6 @@ build_variable ```@docs build_constraint add_constraint -operator_to_set AbstractShape shape reshape_vector @@ -33,4 +32,7 @@ ScalarShape VectorShape SquareMatrixShape SymmetricMatrixShape +operator_to_set +parse_constraint_head +parse_constraint_call ``` diff --git a/src/indicator.jl b/src/indicator.jl index 4063916b4c9..31e17e86859 100644 --- a/src/indicator.jl +++ b/src/indicator.jl @@ -45,7 +45,7 @@ function parse_constraint_call( end rhs_con = rhs.args[1] rhs_vectorized, rhs_parsecode, rhs_buildcall = - parse_constraint(_error, rhs_con) + _parse_constraint(_error, rhs_con) if vectorized != rhs_vectorized _error("Inconsistent use of `.` in symbols to indicate vectorization.") end diff --git a/src/macros.jl b/src/macros.jl index 03cd256c3fe..8d1b318fbde 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -270,7 +270,7 @@ _functionize(x) = x _functionize(::MutableArithmetics.Zero) = 0.0 """ - parse_constraint(_error::Function, expr::Expr) + _parse_constraint(_error::Function, expr::Expr) The entry-point for all constraint-related parsing. @@ -297,11 +297,11 @@ as well as all broadcasted variants. ## See also -The aim of `parse_constraint` is to make this extensible by JuMP extensions. -Thus, `parse_constraint` forwards to [`parse_constraint_head`](@ref) to dispatch -on `expr.head` +The aim of `_parse_constraint` is to make this extensible by JuMP extensions. +Thus, `_parse_constraint` forwards to [`parse_constraint_head`](@ref) to +dispatch on `expr.head` """ -function parse_constraint(_error::Function, expr::Expr) +function _parse_constraint(_error::Function, expr::Expr) return parse_constraint_head(_error, Val(expr.head), expr.args...) end @@ -859,7 +859,7 @@ enable this syntax by defining extensions of user syntax: `@constraint(model, ref[...], expr, my_arg, kw_args...)`. """ macro constraint(args...) - return _constraint_macro(args, :constraint, parse_constraint, __source__) + return _constraint_macro(args, :constraint, _parse_constraint, __source__) end function parse_SD_constraint_expr(_error::Function, expr::Expr) @@ -1012,7 +1012,7 @@ macro build_constraint(constraint_expr) end is_vectorized, parse_code, build_call = - parse_constraint(_error, constraint_expr) + _parse_constraint(_error, constraint_expr) result_variable = gensym() code = quote $parse_code diff --git a/test/constraint.jl b/test/constraint.jl index d6284b8abca..72d5b5c22c4 100644 --- a/test/constraint.jl +++ b/test/constraint.jl @@ -447,8 +447,8 @@ function test_SDP_constraint(ModelType, VariableRefType) err = ErrorException( "In `@SDconstraint(m, [x 1; 1 -y] ⪰ [1 x; x -2], unknown_kw = 1)`:" * " Unrecognized constraint building format. Tried to invoke " * - "`build_constraint(error, JuMP.$(aff_str)[x - " * - "1 -x + 1; -x + 1 -y + 2], JuMP.PSDCone(); unknown_kw = 1)`, but no " * + "`build_constraint(error, $(aff_str)[x - " * + "1 -x + 1; -x + 1 -y + 2], PSDCone(); unknown_kw = 1)`, but no " * "such method exists. This is due to specifying an unrecognized " * "function, constraint set, and/or extra positional/keyword " * "arguments.\n\nIf you're trying to create a JuMP extension, you " * @@ -701,7 +701,6 @@ function test_Model_all_constraints_vector(::Any, ::Any) "`$(GenericAffExpr{Float64})` is not a concrete type. Did you miss a " * "type parameter?", ) - err = ErrorException(replace(err.msg, "JuMP." => "")) @test_throws err try num_constraints( model, From 1d4d8e0fce77eea8a0cc4acbfc85a1730580cb78 Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 7 Oct 2021 15:46:01 +1300 Subject: [PATCH 03/10] Add more docstrings --- src/macros.jl | 122 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 83 insertions(+), 39 deletions(-) diff --git a/src/macros.jl b/src/macros.jl index 8d1b318fbde..7d339ccf50d 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -311,15 +311,39 @@ end Implement this method to intercept the parsing of an expression with head `head`. +!!! warning + Extending the constraint macro at parse time is an advanced operation and + has the potential to interfere with existing JuMP syntax. Please discuss + with the [developer chatroom](https://gitter.im/JuliaOpt/jump-dev) before + publishing any code that implements these methods. + +## Arguments + + * `_error`: a function that accepts a `String` and throws the string as an + error, along with some descriptive information of the macro from which it was + thrown. + * `head`: the `.head` field of the `Expr` to intercept + * `args...`: the `.args` field of the `Expr`. + +## Returns + +This function must return: + + * `is_vectorized::Bool`: whether the expression represents a broadcasted + expression like `x .<= 1` + * `parse_code::Expr`: an expression containing any setup or rewriting code that + needs to be called before `build_constraint` + * `build_code::Expr`: an expression that calls `build_constraint(` or + `build_constraint.(` depending on `is_vectorized`. + +## Existing implementations + JuMP currently implements: - * `::Val{:call}`, which forwards calls to [`parse_constraint_call`](@ref) - * `::Val{:comparison}`, which handles the special case of `l <= body <= u`. + * `::Val{:call}`, which forwards calls to [`parse_constraint_call`](@ref) + * `::Val{:comparison}`, which handles the special case of `l <= body <= u`. -!!! warning - Think carefully before doing this! By implementing this method, your package - will own the sytax. Do not implement a method for syntax that JuMP already - supports. +See also: [`parse_constraint_call`](@ref), [`build_constraint`](@ref) """ function parse_constraint_head(_error::Function, ::Val{T}, args...) where {T} return _error( @@ -330,8 +354,16 @@ function parse_constraint_head(_error::Function, ::Val{T}, args...) where {T} ) end -function parse_constraint_head(_error::Function, ::Val{:call}, args...) - return parse_constraint_call(_error, args...) +function parse_constraint_head( + _error::Function, + ::Val{:call}, + op::Symbol, + args..., +) + op, is_vectorized = _check_vectorized(op) + parse_code, build_call = + parse_constraint_call(_error, is_vectorized, Val(op), args...) + return is_vectorized, parse_code, build_call end function parse_constraint_head( @@ -359,25 +391,25 @@ function parse_constraint_head( "`ub >= expr >= lb` are supported.", ) end - newaff, parse_aff = _MA.rewrite(aff) - newlb, parse_lb = _MA.rewrite(lb) - newub, parse_ub = _MA.rewrite(ub) + new_aff, parse_aff = _MA.rewrite(aff) + new_lb, parse_lb = _MA.rewrite(lb) + new_ub, parse_ub = _MA.rewrite(ub) + parse_code = quote + $parse_aff + $parse_lb + $parse_ub + end build_call = if lvectorized :( build_constraint.( $_error, - _desparsify($newaff), - _desparsify($newlb), - _desparsify($newub), + _desparsify($new_aff), + _desparsify($new_lb), + _desparsify($new_ub), ) ) else - :(build_constraint($_error, $newaff, $newlb, $newub)) - end - parse_code = quote - $parse_aff - $parse_lb - $parse_ub + :(build_constraint($_error, $new_aff, $new_lb, $new_ub)) end return lvectorized, parse_code, build_call end @@ -385,21 +417,39 @@ end """ parse_constraint_call( _error::Function, - vectorized::Bool, + is_vectorized::Bool, ::Val{op}, args..., ) -Parse constraints of the form: -```julia -@constraint(model, op(args...)) -``` -If `vectorized`, the operation is broadcasted, so the constraint is of the form -```julia -@constraint(model, op.(args...)) -``` +Implement this method to intercept the parsing of a `:call` expression with +operator `op`. + +!!! warning + Extending the constraint macro at parse time is an advanced operation and + has the potential to interfere with existing JuMP syntax. Please discuss + with the [developer chatroom](https://gitter.im/JuliaOpt/jump-dev) before + publishing any code that implements these methods. + +## Arguments + + * `_error`: a function that accepts a `String` and throws the string as an + error, along with some descriptive information of the macro from which it was + thrown. + * `is_vectorized`: a boolean to indicate if `op` should be broadcast or not + * `op`: the first element of the `.args` field of the `Expr` to intercept + * `args...`: the `.args` field of the `Expr`. + +## Returns -This function returns a `(parse_code, build_call)` tuple. +This function must return: + + * `parse_code::Expr`: an expression containing any setup or rewriting code that + needs to be called before `build_constraint` + * `build_code::Expr`: an expression that calls `build_constraint(` or + `build_constraint.(` depending on `is_vectorized`. + +See also: [`parse_constraint_head`](@ref), [`build_constraint`](@ref) """ function parse_constraint_call( _error::Function, @@ -415,13 +465,6 @@ function parse_constraint_call( ) end -function parse_constraint_call(_error::Function, operator::Symbol, args...) - operator, vectorized = _check_vectorized(operator) - parse_code, build_call = - parse_constraint_call(_error, vectorized, Val(operator), args...) - return vectorized, parse_code, build_call -end - # `@constraint(model, func in set)` # `@constraint(model, func ∈ set)` function parse_constraint_call( @@ -444,16 +487,17 @@ end parse_constraint_call( _error::Function, vectorized::Bool, - operator::Val, + ::Val{op}, lhs, rhs, - ) + ) where {op} Fallback handler for binary operators. These might be infix operators like `@constraint(model, lhs op rhs)`, or normal operators like `@constraint(model, op(lhs, rhs))`. In both cases, we rewrite as `lhs - rhs in operator_to_set(_error, op)`. + See [`operator_to_set`](@ref) for details. """ function parse_constraint_call( From 9cdad749453187ac26584cc6f9b53171914f7a9f Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 7 Oct 2021 16:07:16 +1300 Subject: [PATCH 04/10] Improve coverage --- src/macros.jl | 2 +- test/macros.jl | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/macros.jl b/src/macros.jl index 7d339ccf50d..6e809e44e00 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -378,7 +378,7 @@ function parse_constraint_head( lsign, lvectorized = _check_vectorized(lsign) rsign, rvectorized = _check_vectorized(rsign) if lvectorized != rvectorized - _error("Signs are inconsistently vectorized") + _error("Operators are inconsistently vectorized.") end if lsign in (:(<=), :≤) && rsign in (:(<=), :≤) # Nothing. What we expect. diff --git a/test/macros.jl b/test/macros.jl index 99a043f3ab1..a3e97db27da 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -1427,6 +1427,43 @@ function test_broadcasting_variable_in_set() return end +function test_parse_constraint_head_error() + model = Model() + @variable(model, x) + err = ErrorException( + "In `@constraint(model, \$(Expr(:(:=), :x, 0)))`: " * + "Unsupported constraint expression: we don't know how to parse " * + "constraints containing expressions of type :=.\n\nIf you are " * + "writing a JuMP extension, implement " * + "`parse_constraint_head(::Function, ::Val{:=}, args...)", + ) + @test_macro_throws(err, @constraint(model, x := 0)) + return +end + +function test_parse_constraint_head_inconsistent_vectorize() + model = Model() + @variable(model, x) + err = ErrorException( + "In `@constraint(model, 1 .<= [x, x] <= 2)`: " * + "Operators are inconsistently vectorized.", + ) + @test_throws(err, @constraint(model, 1 .<= [x, x] <= 2)) + return +end + +function test_parse_constraint_head_inconsistent_signs() + model = Model() + @variable(model, x) + err = ErrorException( + "In `@constraint(model, 1 >= x <= 2)`: " * + "Only two-sided rows of the form `lb <= expr <= ub` or " * + "`ub >= expr >= lb` are supported.", + ) + @test_throws(err, @constraint(model, 1 >= x <= 2)) + return +end + end # module TestMacros.runtests() From 4c6a90fe8c375b9b7b473ab63294ace15b0b2373 Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 7 Oct 2021 16:18:43 +1300 Subject: [PATCH 05/10] File wasn't saved... --- test/macros.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/macros.jl b/test/macros.jl index a3e97db27da..7da4e8c9c21 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -1448,7 +1448,7 @@ function test_parse_constraint_head_inconsistent_vectorize() "In `@constraint(model, 1 .<= [x, x] <= 2)`: " * "Operators are inconsistently vectorized.", ) - @test_throws(err, @constraint(model, 1 .<= [x, x] <= 2)) + @test_macro_throws(err, @constraint(model, 1 .<= [x, x] <= 2)) return end @@ -1460,7 +1460,7 @@ function test_parse_constraint_head_inconsistent_signs() "Only two-sided rows of the form `lb <= expr <= ub` or " * "`ub >= expr >= lb` are supported.", ) - @test_throws(err, @constraint(model, 1 >= x <= 2)) + @test_macro_throws(err, @constraint(model, 1 >= x <= 2)) return end From eec1658f6ea70714d8301b08bd26c1beb0a93b9f Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 7 Oct 2021 16:24:17 +1300 Subject: [PATCH 06/10] Add deprecations --- src/deprecate.jl | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/deprecate.jl b/src/deprecate.jl index 17e459cac9a..e6a8e9e197a 100644 --- a/src/deprecate.jl +++ b/src/deprecate.jl @@ -18,3 +18,7 @@ function value(x, f::Function) @warn("`value(x, f::Function)` is deprecated. Use `value(f, x)` instead.") return value(f, x) end + +@deprecate sense_to_set operator_to_set +@deprecate parse_one_operator_constraint parse_constraint_call +@deprecate parse_constraint_expr _parse_constraint From abd3e9067229dd5b77cef12a72a4b50ffb426924 Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 7 Oct 2021 16:29:46 +1300 Subject: [PATCH 07/10] Reexport parse_constraint --- docs/src/developers/extensions.md | 4 ++++ docs/src/reference/extensions.md | 1 + src/deprecate.jl | 2 +- src/indicator.jl | 2 +- src/macros.jl | 17 +++++++++-------- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/src/developers/extensions.md b/docs/src/developers/extensions.md index 10dadc311d9..29708c927cf 100644 --- a/docs/src/developers/extensions.md +++ b/docs/src/developers/extensions.md @@ -219,6 +219,10 @@ Rewriting my_equal_to to == 2 x = 1.0 ``` +!!! tip + When parsing a constraint you can recurse into sub-constraint (e.g., the + `{expr}` in `z => {x <= 1}`) by calling [`parse_constraint`](@ref). + ### Build To extend the [`@constraint`](@ref) macro at build time, implement a new diff --git a/docs/src/reference/extensions.md b/docs/src/reference/extensions.md index 4d0f0f5771f..22a60c54420 100644 --- a/docs/src/reference/extensions.md +++ b/docs/src/reference/extensions.md @@ -33,6 +33,7 @@ VectorShape SquareMatrixShape SymmetricMatrixShape operator_to_set +parse_constraint parse_constraint_head parse_constraint_call ``` diff --git a/src/deprecate.jl b/src/deprecate.jl index e6a8e9e197a..95ca8fa4e41 100644 --- a/src/deprecate.jl +++ b/src/deprecate.jl @@ -21,4 +21,4 @@ end @deprecate sense_to_set operator_to_set @deprecate parse_one_operator_constraint parse_constraint_call -@deprecate parse_constraint_expr _parse_constraint +@deprecate parse_constraint_expr parse_constraint diff --git a/src/indicator.jl b/src/indicator.jl index 31e17e86859..4063916b4c9 100644 --- a/src/indicator.jl +++ b/src/indicator.jl @@ -45,7 +45,7 @@ function parse_constraint_call( end rhs_con = rhs.args[1] rhs_vectorized, rhs_parsecode, rhs_buildcall = - _parse_constraint(_error, rhs_con) + parse_constraint(_error, rhs_con) if vectorized != rhs_vectorized _error("Inconsistent use of `.` in symbols to indicate vectorization.") end diff --git a/src/macros.jl b/src/macros.jl index 6e809e44e00..71654520087 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -270,7 +270,7 @@ _functionize(x) = x _functionize(::MutableArithmetics.Zero) = 0.0 """ - _parse_constraint(_error::Function, expr::Expr) + parse_constraint(_error::Function, expr::Expr) The entry-point for all constraint-related parsing. @@ -282,6 +282,8 @@ The entry-point for all constraint-related parsing. * `@constraint(model, name[args], expr)` In both cases, `expr` is the main component of the constraint. +## Supported syntax + JuMP currently supports the following `expr` objects: * `lhs <= rhs` * `lhs == rhs` @@ -295,13 +297,12 @@ JuMP currently supports the following `expr` objects: * `!z => {constraint}` as well as all broadcasted variants. -## See also +## Extensions -The aim of `_parse_constraint` is to make this extensible by JuMP extensions. -Thus, `_parse_constraint` forwards to [`parse_constraint_head`](@ref) to -dispatch on `expr.head` +The infrastructure behind `parse_constraint` is extendable. See +[`parse_constraint_head`](@ref) and [`parse_constraint_call`](@ref) for details. """ -function _parse_constraint(_error::Function, expr::Expr) +function parse_constraint(_error::Function, expr::Expr) return parse_constraint_head(_error, Val(expr.head), expr.args...) end @@ -903,7 +904,7 @@ enable this syntax by defining extensions of user syntax: `@constraint(model, ref[...], expr, my_arg, kw_args...)`. """ macro constraint(args...) - return _constraint_macro(args, :constraint, _parse_constraint, __source__) + return _constraint_macro(args, :constraint, parse_constraint, __source__) end function parse_SD_constraint_expr(_error::Function, expr::Expr) @@ -1056,7 +1057,7 @@ macro build_constraint(constraint_expr) end is_vectorized, parse_code, build_call = - _parse_constraint(_error, constraint_expr) + parse_constraint(_error, constraint_expr) result_variable = gensym() code = quote $parse_code From 2178511b63eea5f198cd08ce1d8650c58daeec8d Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 7 Oct 2021 16:56:24 +1300 Subject: [PATCH 08/10] Fix tests --- src/macros.jl | 4 ++-- test/macros.jl | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/macros.jl b/src/macros.jl index 71654520087..40a64d0838f 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -349,9 +349,9 @@ See also: [`parse_constraint_call`](@ref), [`build_constraint`](@ref) function parse_constraint_head(_error::Function, ::Val{T}, args...) where {T} return _error( "Unsupported constraint expression: we don't know how to parse " * - "constraints containing expressions of type $T.\n\nIf you are " * + "constraints containing expressions of type :$T.\n\nIf you are " * "writing a JuMP extension, implement " * - "`parse_constraint_head(::Function, ::Val{$T}, args...)", + "`parse_constraint_head(::Function, ::Val{:$T}, args...)", ) end diff --git a/test/macros.jl b/test/macros.jl index 7da4e8c9c21..327807ec1dd 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -1431,13 +1431,13 @@ function test_parse_constraint_head_error() model = Model() @variable(model, x) err = ErrorException( - "In `@constraint(model, \$(Expr(:(:=), :x, 0)))`: " * + "In `@constraint(model, {x == 0})`: " * "Unsupported constraint expression: we don't know how to parse " * - "constraints containing expressions of type :=.\n\nIf you are " * + "constraints containing expressions of type :braces.\n\nIf you are " * "writing a JuMP extension, implement " * - "`parse_constraint_head(::Function, ::Val{:=}, args...)", + "`parse_constraint_head(::Function, ::Val{:braces}, args...)", ) - @test_macro_throws(err, @constraint(model, x := 0)) + @test_macro_throws(err, @constraint(model, {x == 0})) return end From 4fe42537c1eb803196a2d33ad757bc88231f9d0b Mon Sep 17 00:00:00 2001 From: Oscar Dowson Date: Fri, 8 Oct 2021 16:52:11 +1300 Subject: [PATCH 09/10] Update macros.jl --- test/macros.jl | 1 + 1 file changed, 1 insertion(+) diff --git a/test/macros.jl b/test/macros.jl index 2bfdeb763a8..916c9368f7f 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -1461,6 +1461,7 @@ function test_parse_constraint_head_inconsistent_signs() "`ub >= expr >= lb` are supported.", ) @test_macro_throws(err, @constraint(model, 1 >= x <= 2)) + return end function test_reorder_keyword_arguments() From c1c2940a9e77afe56de496b35f879ee2be025d34 Mon Sep 17 00:00:00 2001 From: Oscar Dowson Date: Wed, 13 Oct 2021 10:15:26 +1300 Subject: [PATCH 10/10] Update docs/src/developers/extensions.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Benoît Legat --- docs/src/developers/extensions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/developers/extensions.md b/docs/src/developers/extensions.md index 29708c927cf..27c58eee381 100644 --- a/docs/src/developers/extensions.md +++ b/docs/src/developers/extensions.md @@ -160,7 +160,7 @@ following methods: publishing any code that implements these methods. [`parse_constraint_head`](@ref) should be implemented to intercept an expression -based on the `.head` field. For example: +based on the `.head` field of `Base.Expr`. For example: ```jldoctest julia> using JuMP