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

[WIP] Handle != constraint via MOI.AllDifferent(2) #3656

Closed

Conversation

LebedevRI
Copy link
Contributor

(@v1.10) pkg> activate /repositories/JuMP.jl
  Activating project at `/repositories/JuMP.jl`

julia> using JuMP
Precompiling JuMP
  1 dependency successfully precompiled in 10 seconds. 37 already precompiled.

julia> model = Model();

julia> @variable(model, x[1:3])
3-element Vector{VariableRef}:
 x[1]
 x[2]
 x[3]

julia> @variable(model, y[1:3])
3-element Vector{VariableRef}:
 y[1]
 y[2]
 y[3]

julia> @constraint(model, x[1] != y[1])
x[1] != y[1]

julia> @constraint(model, x .!= y)
3-element Vector{ConstraintRef{Model, MathOptInterface.ConstraintIndex{MathOptInterface.VectorOfVariables, MathOptInterface.AllDifferent}, VectorShape}}:
 x[1] != y[1]
 x[2] != y[2]
 x[3] != y[3]

julia> @constraint(model, x != y)
ERROR: At REPL[8]:1: `@constraint(model, x != y)`: Ineqality operator with vector operands must be explicitly vectorized, use `.!=` instead of `!=`.
Stacktrace:
 [1] error(::String, ::String)
   @ Base ./error.jl:44
 [2] (::JuMP.Containers.var"#error_fn#98"{String})(str::String)
   @ JuMP.Containers ~/.julia/compiled/v1.10/JuMP/DmXqY_F8XkK.so:-1
 [3] macro expansion
   @ /repositories/JuMP.jl/src/macros/@constraint.jl:132 [inlined]
 [4] macro expansion
   @ /repositories/JuMP.jl/src/macros.jl:393 [inlined]
 [5] top-level scope
   @ REPL[8]:1

I'm not yet sure how to support the not-explicitly vectorized case. We'd need to somehow deduce (in parse_constraint_call()) that our arguments are vectors, and extend parse_constraint_call() to return vectorized itself. I'm not convinced this is even possible.

Otherwise, we get

julia> @constraint(model, x != y)
vectorized = false
ERROR: MethodError: no method matching _build_inequality_constraint(::Bool, ::JuMP.Containers.var"#error_fn#98"{String}, ::Vector{VariableRef}, ::Vector{VariableRef})

Closest candidates are:
  _build_inequality_constraint(::Function, ::Bool, ::Vector{VariableRef}, ::Vector{VariableRef})
   @ JuMP /repositories/JuMP.jl/src/inequality.jl:14

Stacktrace:
 [1] macro expansion
   @ /repositories/JuMP.jl/src/macros/@constraint.jl:132 [inlined]
 [2] macro expansion
   @ /repositories/JuMP.jl/src/macros.jl:393 [inlined]
 [3] top-level scope
   @ REPL[8]:1

(because we should have called @constraint.jl:123)

Missing tests, docs.

As discussed in jump-dev/MathOptInterface.jl#2405

@LebedevRI LebedevRI force-pushed the inequality-as-alldifferent branch 2 times, most recently from 4e0d00f to 37539f3 Compare January 19, 2024 03:35
@LebedevRI LebedevRI changed the title [WIP] parse_constraint_call(): handle Val{:(!=)} via MOI.AllDifferent(2) [WIP] Handle != constraint via MOI.AllDifferent(2) Jan 19, 2024
@LebedevRI LebedevRI force-pushed the inequality-as-alldifferent branch from 37539f3 to bfb59af Compare January 19, 2024 03:39
@odow
Copy link
Member

odow commented Jan 19, 2024

Just add a fallback like

function _build_inequality_constraint(error_fn::Function, ::Bool, lhs, rhs)
    return error_fn(
        "Unsupported form of inequality constraint. The left- and right-hand " *
        "sides must both be decision variables.",
    )
end

Also, just to be explicitly clear before you sink lots of time into this: the wider group of JuMP developers need to have a think about whether we should add a features like !=, and if so, whether this is the right approach. But this is a good starting point for a conversation.

@LebedevRI LebedevRI force-pushed the inequality-as-alldifferent branch from bfb59af to b493e31 Compare January 19, 2024 18:18
@LebedevRI
Copy link
Contributor Author

Now with tests. Any tips for docs?
constraint_string() demonstrates yet another shortcoming of not having a proper set for this.

@LebedevRI LebedevRI force-pushed the inequality-as-alldifferent branch from b493e31 to 8ee39c2 Compare January 19, 2024 18:22
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a55edea) 98.28% compared to head (73f0c9c) 98.29%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3656   +/-   ##
=======================================
  Coverage   98.28%   98.29%           
=======================================
  Files          43       44    +1     
  Lines        5652     5683   +31     
=======================================
+ Hits         5555     5586   +31     
  Misses         97       97           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

```
(@v1.10) pkg> activate /repositories/JuMP.jl
  Activating project at `/repositories/JuMP.jl`

julia> using JuMP
Precompiling JuMP
  1 dependency successfully precompiled in 10 seconds. 37 already precompiled.

julia> model = Model();

julia> @variable(model, x[1:3])
3-element Vector{VariableRef}:
 x[1]
 x[2]
 x[3]

julia> @variable(model, y[1:3])
3-element Vector{VariableRef}:
 y[1]
 y[2]
 y[3]

julia> @constraint(model, x[1] != y[1])
x[1] != y[1]

julia> @constraint(model, x .!= y)
3-element Vector{ConstraintRef{Model, MathOptInterface.ConstraintIndex{MathOptInterface.VectorOfVariables, MathOptInterface.AllDifferent}, VectorShape}}:
 x[1] != y[1]
 x[2] != y[2]
 x[3] != y[3]

julia> @constraint(model, x != y)
ERROR: At REPL[8]:1: `@constraint(model, x != y)`: Ineqality operator with vector operands must be explicitly vectorized, use `.!=` instead of `!=`.
Stacktrace:
 [1] error(::String, ::String)
   @ Base ./error.jl:44
 [2] (::JuMP.Containers.var"#error_fn#98"{String})(str::String)
   @ JuMP.Containers ~/.julia/compiled/v1.10/JuMP/DmXqY_F8XkK.so:-1
 [3] macro expansion
   @ /repositories/JuMP.jl/src/macros/@constraint.jl:132 [inlined]
 [4] macro expansion
   @ /repositories/JuMP.jl/src/macros.jl:393 [inlined]
 [5] top-level scope
   @ REPL[8]:1
```

I'm not yet sure how to support the not-explicitly vectorized case.
We'd need to somehow deduce (in `parse_constraint_call()`)
that our arguments are vectors, and extend `parse_constraint_call()`
to return `vectorized` itself. I'm not convinced this is even possible.

Otherwise, we get
```
julia> @constraint(model, x != y)
vectorized = false
ERROR: MethodError: no method matching _build_inequality_constraint(::Bool, ::JuMP.Containers.var"#error_fn#98"{String}, ::Vector{VariableRef}, ::Vector{VariableRef})

Closest candidates are:
  _build_inequality_constraint(::Function, ::Bool, ::Vector{VariableRef}, ::Vector{VariableRef})
   @ JuMP /repositories/JuMP.jl/src/inequality.jl:14

Stacktrace:
 [1] macro expansion
   @ /repositories/JuMP.jl/src/macros/@constraint.jl:132 [inlined]
 [2] macro expansion
   @ /repositories/JuMP.jl/src/macros.jl:393 [inlined]
 [3] top-level scope
   @ REPL[8]:1
```
(because we should have called `@constraint.jl:123`)

Missing tests, docs.

As discussed in jump-dev/MathOptInterface.jl#2405
@odow
Copy link
Member

odow commented Jan 21, 2024

I've added this to the agenda for the next monthly call (Feb 1). I think it needs a discussion.

@LebedevRI
Copy link
Contributor Author

I've added this to the agenda for the next monthly call (Feb 1). I think it needs a discussion.

Note that i suppose that if there is no decision to support this fully including MOI bits,
this should not proceed, because i don't believe it can be disabled to be reimplemented elsewhere.

@odow
Copy link
Member

odow commented Jan 24, 2024

Yes, if we don't add MOI.NotEqualTo to MOI, then we probably won't add this to JuMP.

You could still add this to LebedevRI/ConstraintProgrammingTools.jl thought. (It's publicly documented https://jump.dev/JuMP.jl/stable/developers/extensions/#Parse). But the warning about clashes applies. If a different extension claimed the Val(:!=) syntax, you wouldn't be able to load both packages at the same time. But that's okay for exploratory packages.

@LebedevRI
Copy link
Contributor Author

Yup.

@LebedevRI
Copy link
Contributor Author

@odow while there, could you comment briefly on the status of SCIP.jl? Is it unmaintained nowadays?

@odow
Copy link
Member

odow commented Jan 24, 2024

SCIP.jl is maintained by the folks at ZIB: https://github.com/scipopt/SCIP.jl. The core JuMP developers do not maintain it and do not have commit access.

@odow
Copy link
Member

odow commented Feb 1, 2024

Hi @LebedevRI, thanks for working on this. We discussed this at length on the monthly developer call.

Thoughts:

  • We understand the desire for this syntax
  • If != lowered to AllDifferent there are a few issues:
    1. the constraint is not what the user typed. This violates some equivalent of the "MethodError principle." I can see people getting confused about why this AllDifferent thing happened when they typed !=
    2. the shape is wrong. The constraint would lower to [x, y] in AllDifferent(2), so things like value(constraint) would return a vector of [value(x), value(y)] instead of value(x != y)
  • The formulation to MILP could be more efficient. this is fixable in MOI, but it is a consideration.
  • There is a high risk that new users will mistakenly use this and end up with models that perform poorly. != is deceptively similar to ==, yet there are very big differences in how they are implemented by solvers.
  • As shown by Add support for NotEqualTo != MathOptInterface.jl#2405 (comment), you could implement this as a third-party extension

Conclusions:

  • We will not add != support to JuMP (for now)
  • We recommend that users do [x, y] in MOI.AllDifferent(2) instead
  • We recommend that interested developers start by implementing a JuMP extension, following Add support for NotEqualTo != MathOptInterface.jl#2405 (comment)
  • If the extension demonstrates utility and value, we may reconsider.

I'll post in jump-dev/MathOptInterface.jl#2405 as well with similar/related conclusions.

@odow odow closed this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants