Update tests with add_constrained_variable(s)

[blegat]

Use add_constrained_variable and add_constrained_variables instead of add_variable/add_variables in a few tests.
Also create a new rotated SOC test.
There is no rule on whether one should use add_variable or add_constrained_variable in the tests except in some places where using add_constrained_variable would not work as the test use modification not supported by the bridge.
The tests are run with an SDPA model in test/Bridges/lazy_bridge_optimizer.jl to make sure tests don't do the wrong choice in #759

Extracted from #759

[mlubin]

There is no rule on whether one should use add_variable or add_constrained_variable in the tests except in some places where using add_constrained_variable would not work as the test use modification not supported by the bridge.

This is an issue, because people reading the tests and writing the tests won't know which one to use. Could we test add_constrained_variable more systematically?

[blegat]

It's kind of similar with VectorOfVariables vs SingleVariable and VAF vs SAF were there is no rulewhich one to use.
Would adding one unit test for add_constrained_variable and one for add_constrained_variables resolve the issue ?

[codecov-io]

Codecov Report

Merging #803 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
+ Coverage   94.09%   94.21%   +0.11%     
==========================================
  Files          59       59              
  Lines        6795     6892      +97     
==========================================
+ Hits         6394     6493      +99     
+ Misses        401      399       -2

Impacted Files	Coverage Δ
src/Test/modellike.jl	99.32% <100%> (+0.1%)	⬆️
src/Test/contconic.jl	98.14% <100%> (+0.07%)	⬆️
src/Utilities/mockoptimizer.jl	89.65% <0%> (-0.5%)	⬇️
src/Bridges/bridge_optimizer.jl	91.82% <0%> (+0.05%)	⬆️
src/Utilities/model.jl	93.5% <0%> (+0.28%)	⬆️
src/Bridges/Constraint/interval.jl	92.1% <0%> (+5.26%)	⬆️

---

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 2c0e0fc...a733d97. Read the comment docs.

[mlubin]

Would adding one unit test for add_constrained_variable and one for add_constrained_variables resolve the issue ?

Adding these tests would be good, but this doesn't address the issue of add_constrained_variable being used seemingly arbitrarily in the other tests. We need to be able to give a reason for why people should use add_variable vs. add_constrained_variable when writing MOI code. The only reason I'm aware of is to enable bridges, so we should focus on cases where bridges are useful, like

MOI.add_constrained_variables(model, MOI.RotatedSecondOrderCone(4))

Are there any solvers for which a bridge would apply to:

MOI.add_constrained_variable(model, MOI.LessThan(0.0))

?

[blegat]

Yes, SDP need variable bridges for any variable that's not added with add_constrained_variables with Nonnegatives or PSD.

[mlubin]

Yes, SDP need variable bridges for any variable that's not added with add_constrained_variables with Nonnegatives or PSD.

What does this mean for variables with both lower and upper bounds?

[blegat]

One of the bounds will be transformed to an affine constraint via the Constraint.FunctionizeBridge and then the SlackBridge will create another variable. For interval and free variables. You need to create to variables in SDP solvers

[mlubin]

What I don't understand is the asymmetry. If you want to impose LessThan(10.0) and GreaterThan(1.0) (e.g., as would be generated by @variable(m, lower_bound=1, upper_bound=10)), how should you write it?

How will SDP solvers be able to handle the following:

x = add_variable(model)
add_constraint(model, x, LessThan(10.0))
add_constraint(model, x, GreaterThan(1.0))

?
What do you gain from add_constrained_variable if the above code works?

[blegat]

With:

x = add_variable(model)
add_constraint(model, x, LessThan(10.0))
add_constraint(model, x, GreaterThan(10.0))

you first create a free variables which is bridged into one nonnegative + one nonpositive using the FreeBridge and then the Nonpositive is bridged to Nonnegative using the FlipSignBridge.
Then x corresponds to an affine expression of solver variables so the two following constraints are bridged to ScalarAffineFunction constraint using the Constraint.FunctionizeBridge. Then you need to create one slack for each constraint.
So in total, you create 4 nonnegative variables and 2 equality constraints.

If you write

x = add_constrained_variable(model, LessThan(10.0))
add_constraint(model, x, GreaterThan(10.0))

you only create 2: the nonnegative variables 10 - x and the slack variable for the constraint. And you only create one quality constraint.

[mlubin]

you first create a free variables which is bridged into one nonnegative + one nonpositive using the FreeBridge and then the Nonpositive is bridged to Nonnegative using the FlipSignBridge.

Minor: this seems wasteful since I'm not aware of solvers that that don't support free variables but do support nonnegative variables. Could you bridge them into a difference of two nonpositive variables and save an extra transformation?

Ok, I see why add_constrained_variable is useful for solvers that don't support free variables. If you know that a variable is constrained on one side, you don't need to create the positive and negative parts. However, why do we need to use add_constrained_variable in these tests? Do any of these solvers support direct mode? If not, won't copy_to greedily pick either LessThan or GreaterThan and make the variable constrained when it's copied from the cache?

[blegat]

Could you bridge them into a difference of two nonpositive variables and save an extra transformation?

Transforming into an nonnegatives and a nonpositives seems cleaner as it does not favor any cone. As it's for SDP solver, saving a variable bridge shouldn't be critical for performance.

However, why do we need to use add_constrained_variable in these tests?

There is no need as they support free variables. There is no strict reason to use it or not use it. Just like with VectorAffine vs ScalarAffine, we should do a bit of both to have good coverage.

Do any of these solvers support direct mode?

(SDPT3 will probably do but the wrapper is not done yet, I was waiting for variable bridges.) However, even if you use a bridged followed by a caching optimizer, since LazyBridgeOptimizer decides whether it bridges or not only depending on supports_constraint, using a cache or not won't change anything.
If we do cache then bridges then cache, however, the bridge won't see the same variable calls than what's given in the tests but that's not what is currently done for conic tests.

[mlubin]

Just like with VectorAffine vs ScalarAffine, we should do a bit of both to have good coverage.

This should be done systematically. It should be clear to a reader what each tests is designed to cover. Sprinkling add_constrained_variable a bit here and a bit there creates a big maintenance issue, because it will be difficult to modify or refactor these tests while being confident that they cover the same functionality.

[blegat]

I agree but this is how contlinear, ... started (on the contrary to unit tests where thigs are much cleaner) so this PR is consistent with the current tests. So I would move this to a separate issue to refactor these tests to have a systematic way to choose among the different way the model can be written.

The current rule could be: use add_constrained_variables if the variable is not free unless that prevents
https://github.com/JuliaOpt/MathOptInterface.jl/pull/759/files#diff-898e033f09a47e463470f965c88c3ba2R147
from passing

[mlubin]

Transforming into an nonnegatives and a nonpositives seems cleaner as it does not favor any cone.

It's natural to favor nonnegatives because there are zero solvers that support nonpositives and do not support nonnegatives.

If we do cache then bridges then cache, however, the bridge won't see the same variable calls than what's given in the tests but that's not what is currently done for conic tests.

What if we make it easier to run the tests with cache then bridges then cache? This is the default configuration for JuMP, so we'd be testing out the same pathway that JuMP uses. Then there's no need to use add_constrained_variables in the tests explicitly because they would be covered by the cache then bridges then cache tests.

I agree but this is how contlinear, ... started

Yes, but adding on top of a mess only makes it worse. I'm looking for better alternatives. If we really can't find any, then I would be satisfied with additional comments and a new issue opened.

[blegat]

It's natural to favor nonnegatives because there are zero solvers that support nonpositives and do not support nonnegatives.

Ok let's change that in the bl/variable_bridges PR.

What if we make it easier to run the tests with cache then bridges then cache? This is the default configuration for JuMP, so we'd be testing out the same pathway that JuMP uses.

Ok so say that the tests try to mirror what JuMP would call, and would also show what's recommended. So given a @variable, in JuMP 0.20, it will

• If it has no bounds and is not integer: call add_variable
• if it has no bound but is integer: call add_constrained_variable with MOI.Integer
• if it has bounds: call add_constrained_variable with bounds and then enventually set it to integer with add_constraint.

If a solver need caches to passes the tests, he's responsible to add them. It may need cache+bridge+cache as with JuMP.

[mlubin]

if it has no bound but is integer: call add_constrained_variable with MOI.Integer
if it has bounds: call add_constrained_variable with bounds and then enventually set it to integer with add_constraint.

Hmm. How can we support dropping integrality or removing bounds as we do currently if JuMP created variables with add_constrained_variable? I was only thinking that JuMP would use add_constrained_variable for special conic sets like PSD or RSOC. (Users don't expect to drop PSD-ness from a variable. They do expect to be able to drop and add bounds.)

If a solver need caches to passes the tests, he's responsible to add them.

Solver authors really shouldn't need to understand cache+bridge+cache in order to test their solver. We should provide a few modes of testing, that you can choose easily from, like direct mode, bridge+cache, and cache+bridge+cache.

[odow]

I feel like we need some sort of requires_add_constrained_variable(model, set). Because it seems like in general, we should just use add_variable and add_constraint. And only sometimes (e.g., Mosek PSD) do we want to use add_constrained_variable. There are too many confusing edge-cases about when to use which way, and the way forward seems to be to pass that decision making onto the solvers for them to declare which one they support.

I'd even like a function

function how_to_support_constrained_variable(model, sets...)
    normal_constraint = []   # Sets that should be supported using add_constraint
    constrained_variable = []  # Sets that should be supported using add_constrained_varaible
    # ...
    return normal_constraint, constrained_variable
end

[blegat]

How can we support dropping integrality or removing bounds as we do currently if JuMP created variables with add_constrained_variable ?

Yes, it's just doing two operations (add_variable and add_constraint) in one.

I feel like we need some sort of requires_add_constrained_variable(model, set).
Because it seems like in general, we should just use add_variable and add_constraint.

In general, you should use add_constrained_variable. By calling the two operations in one, you allow the MOI models to do more things (e.g. Mosek PSD variables and variable bridges), there is no case where calling the two separately is preferable.

[mlubin]

In general, you should use add_constrained_variable. By calling the two operations in one, you allow the MOI models to do more things (e.g. Mosek PSD variables and variable bridges), there is no case where calling the two separately is preferable.

What happens when you use add_constrained_variable and then delete the constraint? Wouldn't that break the transformations implemented by the bridges?

I think there's a conceptual mismatch here that accounts for some of the confusion. We think of SingleVariable constraints almost like attributes that you can set, get, remove, add, etc. at any time. This is how they're now implemented in Model, and this is how most solvers and users think about them.

If you think of SingleVariable constraints like this, asking users to choose one particular SingleVariable constraint to provide at variable creation time (and leaving this as a choice) is a confusing API design. There are other ways to address the issue of handling free variables in solvers that support only nonnegative variables (e.g., look at both variable bounds and then decide what to do). add_constrained_variable makes a lot of sense for VectorOfVariables constraints, but less so for SingleVariable constraints.

It might help to have an offline discussion.

[blegat]

Just changed the PR. It only modifies contconic.jl where it uses add_constrained_variables to constrain vector of variables.
In modellike.jl, it creates variables using all techniques to ensure good coverage