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

Clean up and document parse_constraint #2236

Closed
blegat opened this issue May 5, 2020 · 6 comments · Fixed by #2736
Closed

Clean up and document parse_constraint #2236

blegat opened this issue May 5, 2020 · 6 comments · Fixed by #2736

Comments

@blegat
Copy link
Member

blegat commented May 5, 2020

We might consider renaming parse_constraint in parse_constraint_call and parse_constraint_head in parse_constraint but this might be breaking. We can consider this renaming just before we plan to release JuMP v0.22 or v1.0.
Another option is to just remove the current parse_constraint and rename parse_constraint_head in parse_constraint. Indeed, implementing

parse_constraint_call(_error::Function, [...])

or

parse_constraint(_error::Function, ::Val{:call}, [...])

is not too different and it might be confusing to have many different function names while only one would be enough.

See #2228

@odow
Copy link
Member

odow commented Oct 3, 2021

There's a lot of complexity here. I've been reading through this all morning, along with the old issues.

How much are we willing to break? It seems like there could be a cleaner solution than all of these parse_constraint_head, parse_constraint, parse_constraint_expr, parse_one_operator_constraint, parse_ternary_constraint, etc.

But that's going to be a hard break for every extension.

Here were there packages I found using these extensions using JuliaHub (e.g., https://juliahub.com/ui/Search?q=parse_one_operator_constraint&type=code&w=true):

  • parse_constraint: JuCP (@dourouc05)
  • parse_one_operator_constraint: ConstraintSolver (@Wikunia), SetProg (@blegat)
  • parse_ternary_constraint: JuCP (@dourouc05)
  • parse_constraint_expr: ConstraintSolver (@Wikunia)
  • parse_constraint_head: ConstraintSolver (@Wikunia)

Am I missing others?

I think it would be useful if the people tagged could post below with the syntax they currently support, and the syntax they would like to support. Then we can work backward from there to a better design.

@odow odow changed the title Clean up parse_constraint Clean up and document parse_constraint Oct 4, 2021
@dourouc05
Copy link
Contributor

parse_constraint in ConstraintProgrammingExtensions is a false positive, it's only an internal method for parsing FZN files (https://github.com/dourouc05/ConstraintProgrammingExtensions.jl/search?q=parse_constraint).

Syntax-wise, I had a few examples in #2227. For the record, here it is:

@constraint(m, f(x)) # Examples: all_different(x), x (equivalent to x == true)
@constraint(m, x := { constraint(x, y) } # Reification. Example: x := { y >= 0 }
# Both for JuCP and ConstraintSolver: https://wikunia.github.io/ConstraintSolver.jl/dev/supported/#Anonymous-variables
# Really similar to indicator constraints
@constraint(m, x == count(y .== 1))
# Both for JuCP and ConstraintSolver: https://github.com/Wikunia/ConstraintSolver.jl/pull/213
@constraint(m, x == y[z]) # Either y or z being variable(s)... or even both

# Then, add Boolean operators into the mix
@constraint(m, x \wedge y) # Meaning that both x == true and y == true (alternative LaTeX name: \land)
@constraint(m, x & y) # Alternative syntax
@constraint(m, { x <= 0} \vee { y <= 0} \vee {z == 0}) # Disjunction
@constraint(m, x := { x >= 0 \vee y >= 0 } # Reification of disjunction

@odow
Copy link
Member

odow commented Oct 4, 2021

The problem with the current approach is that extensions are going to fight over the same syntax. We probably want something like

@constraint(model, x == y[z], JuCP())

where we can intercept not just the build_constraint argument, but also a parse_constraint.

Otherwise I don't see how we can ever distinguish x== y[z] from a EqualTo constraint.

@dourouc05
Copy link
Contributor

The difference between a simple ´EqualTo´ with a constant and an array indexing is that, for array indexing, the index is variable. This is really easy to do with multiple dispatch, but not at all at the macro level… (At some point, I was thinking about generated methods with @generated, but I'm not sure it's such a good idea.) The real problem here, I think, is to maintain a really fast code path for the most common cases (getting a constant from an array, getting a variable at a constant position).

To avoid the need for another argument, couldn't dispatch be made on the type of model?

Also, I think there is some hope that packages won't clash too much over the syntax: ConstraintSolver and JuCP provide similar syntax, but in the end only one package out of the two will implement it (and ConstraintSolver will have JuCP as dependency). Most importantly, the semantics are not changing at all between the packages. I don't think that clashes should be considered first for the new design (but, if they can be avoided by some feature like you proposed, that would be best).

@blegat
Copy link
Member Author

blegat commented Oct 5, 2021

Yes, I also don't think we should worry too much about the clashes either

@Wikunia
Copy link
Contributor

Wikunia commented Oct 5, 2021

Let me list the current useage of all of them in ConstraintSolver.jl

  • parse_one_operator_constraint is used for boolean constraints so x == 1 && y == 1 (also with || and \xor)
    It's also used for complement constraints like !(x == 1 && y == 1).
  • parse_constraint_expr is simply called from my solver I don't implement any methods for this
  • parse_constraint_head is also used for boolean constraints. I'm not quite sure why I implemented both though. The one above takes vectorized as an argument but I only allow unvectorized versions anyway.
    It's also used for reified constraints so b := {x == 1} so just := instead of => which is used for indicator constraints.

I'm fine with the current version to be honest. I also don't think that different users should specify which implementation to use based on the solver. Hopefully every constraint programming solver will eventually use the code by @dourouc05 . It would also make switching between solvers more difficult I guess.

Dispatching on model is probably not going to work as the solver is not known at that stage, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants