ListOfConstraints -> ListOfConstraintTypes

[odow]

Closes #214

I also propose changing

 A list of tuples of the form `(F,S)`, where `F` is a function type
 and `S` is a set type indicating that the attribute `NumberOfConstraints{F,S}()`
 has value greater than zero.

to

 A list of tuples of the form `(F,S)`, where `F` is a function type
 and `S` is a set type indicating that `supportsconstraint(model, F, S)` is `true`.

Updated: changed cansupport to supportsconstraint.

[codecov-io]

Codecov Report

Merging #357 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #357   +/-   ##
=======================================
  Coverage   96.27%   96.27%           
=======================================
  Files          36       36           
  Lines        4723     4723           
=======================================
  Hits         4547     4547           
  Misses        176      176

Impacted Files	Coverage Δ
src/Utilities/model.jl	98.71% <ø> (ø)	⬆️
src/Test/contconic.jl	100% <100%> (ø)	⬆️
src/Test/modellike.jl	100% <100%> (ø)	⬆️
src/Utilities/functions.jl	96.92% <100%> (ø)	⬆️
src/attributes.jl	93.61% <100%> (ø)	⬆️
src/Test/intconic.jl	100% <100%> (ø)	⬆️
src/Utilities/copy.jl	90.47% <100%> (ø)	⬆️
src/Bridges/bridgeoptimizer.jl	98.52% <100%> (ø)	⬆️

---

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 e85ea35...503abf6. Read the comment docs.

[mlubin]

The change looks good to me.
By cansupport do you mean canaddconstraint or supportsconstraint? This is really a mess (#295).

[odow]

supportsconstraint.

This is really a mess

Illustrated by the fact I've just spent the last few days writing tests for the MOI API and I mis-remembered the name of the method...

[odow]

canaddconstraint has a clear, concise meaning. It should stay.

Maybe ListOfConstraintTypes should be changed to ListOfSupportedConstraintTypes, and supportsconstraint should be removed, in favor of (F,S) in get(m, ListOfSupportedConstraintTypes())

Then there is a clear separation between "can I add right now" and "do you even support this constraint type."

[mlubin]

Agreed that would be an improvement from what we do now.

[blegat]

(F,S) in get(m, ListOfSupportedConstraintTypes()) would be inefficient if get returns a Vector. Why would we change it to the list of constraint types supported ? This is not possible with the UniversalFallback since it would support an infinite number of constraint types :-P

[odow]

https://github.com/JuliaOpt/MathOptInterface.jl/blob/f238c74fa323e3e1f29d45bf11fc4c63de169e77/src/Utilities/universalfallback.jl#L152-L153

Surely supportsconstraint is only true if uf.model supports it?

would be inefficient if get returns a Vector

The type stable way is to check canaddconstraint. supportsconstraint could/should be used by extensions etc to check if the solver supports a constraint. For example, in SDDP.jl, I add a ScalarAffineFunction-in-EqualTo constraint without telling the user. I might want to check that the solver supports such a constraint at the start before I start building the models.

[mlubin]

There is a point there that having an explicit list of all possible constraint types supported could be problematic and even infinite for UniversalFallback.

[odow]

How about renaming supportsconstraint to isconstrainttypesupported.

Long and verbose (and has an ugly tt), but at least it is explicit enough to clear up the confusion.

Other options

• is_constraint_type_supported
• get(m, IsSupportedConstraintType(), F, S)

[blegat]

Surely supportsconstraint is only true if uf.model supports it?

No it supports everything, if what is not supported by uf.model. Currently UniversalFallback only implements attribute fallback so looking at the source code will not help :-P

The type stable way is to check canaddconstraint

canaddconstraint should be fast since it is used by CachingOptimizer and supportsconstraint should also be fast since it is used by LazyBridgeOptimizer (which is coming in the next few days).

How about renaming supportsconstraint to isconstrainttypesupported.

Why would it be less confusing ? The distinction that should be made is:

• canaddconstraint : The constraint can be added now in the current state of the model
• supportsconstraint : The model supports the constraint independently of it state

It seems to me that the canadd and supports wording convey the right distinction.

[mlubin]

canaddconstraint: The constraint can be added now in the current state of the model

I'm going to re-suggest (#295 (comment)) that we remove this concept completely. If a model claims to support a constraint independently of its state, but for some reason the constraint cannot be added right now, I would like to see an error message on addconstraint rather than a false without an explanation.

[blegat]

Copy-only solvers will return false to canaddconstraint and true to supportsconstraint and the CachingOptimizer uses this fact to determine that the solver needs to go to the EmptyState. Having a try catch to handle this would be less efficient and less elegant I'd say

[mlubin]

Copy-only solvers will return false to canaddconstraint and true to supportsconstraint

#295 (comment)

[blegat]

#295 (comment)

#295 (comment) :D

[mlubin]

Some solvers might, after optimize! is called, support adding some kind of constraints but not other kind of constraints, even if they support these constraints.

That's fine, they can return errors in that case, and users can work around them. The extra complexity in the API is not worth it, IMO.

[blegat]

The extra complexity in the API is not worth it, IMO.

We may keep the current MOI.canaddconstraint and add MOI.canaddconstraint(::MOI.ModelLike) which returns a Bool indicating whether any supported constraint can be added.
A direct consequence of this definition gives the following valid default implementation

MOI.canaddconstraint!(m::ModelLike, F::Type{<:AbstractFunction}, S::Type{<:AbstractSet}) = MOI.canaddconstraint!(m) && MOI.supports

Most solvers will only define MOI.canaddconstraint!(::ModelLike) but for a solver for which this depends on the constraint types, it can overwrite this default implementation so that the CachingOptimizer takes its particularity into account.

[mlubin]

it can overwrite this default implementation so that the CachingOptimizer takes its particularity into account.

Could you walk through a realisitc example of why this is important and what would go wrong if CachingOptimizer just uses MOI.canaddconstraint(m) && MOI.supports(m, F, S)?

[blegat]

Suppose that once optimize! has been called, a mixed integer solver supports adding linear cuts after optimize! (i.e. ScalarAffineFunction-in-LessThan{Float64}) but does not support changing the integrality of the variables (i.e. SingleVariable-in-Integer and SingleVariable-in-Reals).
When the user change the integrality through JuMP, the CachingOptimizer should empty the solver but then a linear constraint is added, the CachingOptimizer should just add the constraint (if it empties the solver, there will be a big performance loss since the optimization needs to be restarted from scratch).
The solver will do

MOI.supportsconstraint(m::MySolver, ::Type{<:ScalarAffineFunction}, S::Type{<:LessThan}) = true
MOI.supportsconstraint(m::MySolver, ::Type{<:SingleVariable}, S::Type{<:Integer}) = true
MOI.canaddconstraint(m::MySolver) = !m.optimized
MOI.canaddconstraint(m::MySolver, ::Type{<:ScalarAffineFunction}, S::Type{<:LessThan}) = true
MOI.canaddconstraint(m::MySolver, ::Type{<:SingleVariable}, S::Type{<:Integer}) = !m.optimized

[mlubin]

I think it's perfectly fine for the CachingOptimizer to attempt to change the integrality of the variables and then return an error in that case. Users will just need to manually reset the solver.

[blegat]

The user could be an algorithm and the algorithm could be solver independent where some solvers support the action and some do not. If reset is not called, the algorithm will fail for some solver and if reset is called, the algorithm will unnecessarily reset some solver.

[mlubin]

There's a reason why there's not a single other API that tries to solve this problem. It's hard; we have to make trade-offs at some point.
MOI.canaddconstraint(m::MySolver, F, S) has a high cost to support.

• It is confusing for solver authors (as evidenced by the multiple issues on it).
• It induces hard-to-explain behavior in CachingOptimizer. ("Why did it just drop the solver?")
• It obscures error messages that the user might really want to see.

Overall not worth it to me.

If reset is not called, the algorithm will fail for some solver and if reset is called, the algorithm will unnecessarily reset some solver.

If you want to be on the safe side, always reset the solver. It's ok really. You'll still be able to publish your paper. If that's not good enough, use direct mode and throw errors if the user provides a solver that doesn't support the functionality you need. If that's not good enough, write your own version of caching optimizer that's suited for your application.

[mlubin]

FWIW if you want to rename canaddconstraint to should_caching_optimizer_try_to_add_constraint then go ahead. That would at least be more honest :)

[blegat]

I agree that they are tradeoffs to be made. In the use case I was mentionning can still be solved by the solver caching the integrality changes internally. It is more work but there is also the cost of having canaddconstraint that you have just mentionned.

If everyone agrees on replacing canaddconstraint(::MOI.ModelLike, F, S) by canaddconstraint(::MOI.ModelLike) I won't oppose it.
@joaquimg @odow What do you think ?

[odow]

I don't have a problem with canaddconstraint. It has a clear concept, and lines up with the rest of the canXXX API.
canaddconstraint returns false and you're not sure why? Call addconstraint! to throw the error.

• canaddconstraint : The constraint can be added now in the current state of the model
• supportsconstraint : The model supports the constraint independently of it state

I would change the second definition to "The model supports the constraint type independently of it state"

Therefore, what about renaming supportsconstraint to supportsconstrainttype.

Maybe also have a fallback so canaddconstraint(m, F, S) = supportsconstraintype(m, F, S).

[mlubin]

It has a clear concept, and lines up with the rest of the canXXX API.

I'm also proposing to dump the whole canXXX API, or at least rename it to should_caching_optimizer_try_to_do_XXX because that's what it really is.

[blegat]

• It induces hard-to-explain behavior in CachingOptimizer. ("Why did it just drop the solver?")
• It obscures error messages that the user might really want to see.

The Manual mode of the CachingOptimizer is the solution for both of these issues

I'm also proposing to dump the whole canXXX API, or at least rename it to should_caching_optimizer_try_to_do_XXX because that's what it really is.

The first solution would kill the Automatic mode of the CachingOptimizer and I don't see how the second solution would make things clearer. canget has a self-contained definition independent on the CachingOptimizer, mentioning the CachingOptimizer in the name wouldn't make it clearer IMO. Adding a note in the definition may help though.

[mlubin]

canget has a self-contained definition independent on the CachingOptimizer

It's a problematic definition however. The issue is that canXXX leads CachingOptimizer to hide errors that the user might want to see (#302). Manual mode is sometimes but not always the answer to this (it doesn't resolve #302).

The difference between canXXX and should_caching_optimizer_try_to_do_XXX is that the latter should return true also in the case that XXX is known to return an error that the user should see.

[chriscoey]

what's the status of this particular PR?

[odow]

@blegat is probably the best person to comment on this. I'm in favor of ListOfConstraints -> ListOfConstraintTypes, but I'm not sure what the definition should be.

[blegat]

The renaming is agreed upon but the change of definition would not work because of UniversalFallback for instance. What are the advantages of changing the definition ?

[mlubin]

I don't see a pressing need to change the definition. I support a renaming, maybe to ListOfConstraintsPresent.

[odow]

Closing this as stale. I'll leave the issue open for now.