Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[breaking] refactor the parse_constraint methods #2736

Merged
merged 12 commits into from
Oct 12, 2021
Merged

Conversation

odow
Copy link
Member

@odow odow commented Oct 7, 2021

Background

We previously had

  • parse_constraint_expr
  • parse_constraint_head
  • parse_one_operator_constraint
  • parse_ternary_constraint
    with a mix of interceptions and reorganizations.

I've simplified this to:

  • A single parse_constraint entry point from the macro. This is not for extending.
  • parse_constraint_head, which is for intercepting Expr(head, ...) expressions.
  • parse_constraint_call, which is for intercepting Expr(:call, op, ...) expressions.

These are now better documented, and the documentation has examples of extending via parse_constraint_.

Deprecations

  • sense_to_set is now operator_to_set
  • parse_one_operator_constraint is now parse_constriant_call
  • parse_constraint_expr is now parse_constraint

Breaking changes

  • parse_ternary_constraint is removed

TODOs

Closes #2236

@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #2736 (c1c2940) into master (6a790f8) will increase coverage by 0.19%.
The diff coverage is 98.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2736      +/-   ##
==========================================
+ Coverage   93.90%   94.10%   +0.19%     
==========================================
  Files          44       43       -1     
  Lines        5561     5512      -49     
==========================================
- Hits         5222     5187      -35     
+ Misses        339      325      -14     
Impacted Files Coverage Δ
src/deprecate.jl 100.00% <ø> (ø)
src/macros.jl 95.76% <97.95%> (+0.58%) ⬆️
src/complement.jl 100.00% <100.00%> (ø)
src/indicator.jl 100.00% <100.00%> (ø)
src/Containers/generate_container.jl
src/quad_expr.jl 97.82% <0.00%> (+0.02%) ⬆️
src/aff_expr.jl 96.00% <0.00%> (+0.14%) ⬆️
src/Containers/DenseAxisArray.jl 88.94% <0.00%> (+0.17%) ⬆️
src/Containers/macro.jl 96.66% <0.00%> (+0.27%) ⬆️

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 6a790f8...c1c2940. Read the comment docs.

@odow odow changed the title WIP: refactor the parse_constraint methods RFC: refactor the parse_constraint methods Oct 7, 2021
@odow odow added breaking Category: Extensions Related to JuMP extensions Category: Internals Related to JuMP internals labels Oct 7, 2021
@odow odow added this to the 1.0 milestone Oct 7, 2021
@odow
Copy link
Member Author

odow commented Oct 7, 2021

@dourouc05 Here's an example of the binpacking from #2246

julia> using JuMP

julia> const MutableArithmetics = JuMP._MA;

julia> function JuMP.parse_constraint_call(
           _error::Function,
           is_vectorized::Bool,
           ::Val{:binpacking},
           args...,
       )
           @assert !is_vectorized
           new_args = MutableArithmetics.rewrite.(args)
           parse_code = quote end
           new_lhs = Expr(:vect)
           for (lhs, code) in new_args
               push!(parse_code.args, code)
               push!(new_lhs.args, lhs)
           end
           N = length(args)
           x = gensym()
           push!(parse_code.args, Expr(:(=), x, new_lhs))
           build_code = :(build_constraint($(_error),  $new_lhs,  MOI.Zeros($N)))
           return parse_code, build_code
       end

julia> model = Model(); @variable(model, x[1:4]);

julia> @constraint(model, binpacking(x[1], x[2], x[3], x[4]))
[x[1], x[2], x[3], x[4]]  MathOptInterface.Zeros(4)

@odow odow changed the title RFC: refactor the parse_constraint methods RFC: [breaking] refactor the parse_constraint methods Oct 7, 2021
@dourouc05
Copy link
Contributor

It looks nicer than the current solution! I think that the only type of syntax that cannot be implemented is array indexing, which is tricky anyway.

Is there any standard error message for constraints that do not support vectorisation? (To have a uniform message instead of a raw @assert !is_vectorized.)

@odow
Copy link
Member Author

odow commented Oct 7, 2021

You could implement the vectorized version. I just didn't here for simplicity.

Array indexing is very tricky because it isn't syntactically unique. Instead it depends on the data. You're best to have

@constraint(model, x == element(y, z))

or something similar

@odow odow requested a review from blegat October 7, 2021 18:50
@dourouc05
Copy link
Contributor

You could implement the vectorized version. I just didn't here for simplicity.

That's for that specific example. What about other constraints where vectorisation does not make sense at all? I'm thinking about Membership, which could have a syntax like x in [a, b, c], which parses like in(x, [a, b, c]). Calling in. isn't really intuitive and I don't feel like supporting in.(items, Ref(collection)).

@odow
Copy link
Member Author

odow commented Oct 7, 2021

Then just throw an informative error. I don't think we need a specific one for now.

@odow
Copy link
Member Author

odow commented Oct 10, 2021

@blegat can you take a look at this?

@odow odow changed the title RFC: [breaking] refactor the parse_constraint methods [breaking] refactor the parse_constraint methods Oct 10, 2021
@odow odow merged commit 86da6f3 into master Oct 12, 2021
@odow odow deleted the od/parse-constraint branch October 12, 2021 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Category: Extensions Related to JuMP extensions Category: Internals Related to JuMP internals
Development

Successfully merging this pull request may close these issues.

Clean up and document parse_constraint
3 participants