Add tests, extend docstring for ScalarQuadraticFunction.

[rschwarz]

We just discovered a bug in SCIP.jl (#106) in the handling of quadratic coefficients off the diagonal, based on my misunderstanding of the MOI docs.

To add prevent this kind of problem, I've added a small example to the docstring and two QCP tests.

[rschwarz]

By the way, I don't know the process for adding tests to MOI. Should these pass with some subset of solvers? Is this considered a breaking change that requires a major version bump?

[blegat]

It's not considered a breaking change. The solvers should be passing the new tests, otherwise it means they have a bug.

[rschwarz]

The solvers should be passing the new tests, otherwise it means they have a bug.

That makes sense, but it also requires that tests should be highly configurable in terms of what the solvers support. For example, SCIP.jl excludes one test of the QCP suite, because it requires VectorAffineFunctions. If somebody added another test of that kind, it would fail on SCIP.jl, but I would not consider that as evidence for a bug.

[codecov-io]

Codecov Report

Merging #697 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #697      +/-   ##
=========================================
- Coverage   95.23%   95.2%   -0.04%     
=========================================
  Files          53      54       +1     
  Lines        5546    5690     +144     
=========================================
+ Hits         5282    5417     +135     
- Misses        264     273       +9

Impacted Files	Coverage Δ
src/functions.jl	100% <ø> (ø)	⬆️
src/Test/contquadratic.jl	100% <100%> (ø)	⬆️
src/modifications.jl	50% <0%> (-7.15%)	⬇️
src/Bridges/slackbridge.jl	94.82% <0%> (-5.18%)	⬇️
src/sets.jl	95.74% <0%> (-4.26%)	⬇️
src/Bridges/detbridge.jl	98.75% <0%> (-1.25%)	⬇️
src/Test/intlinear.jl	98.95% <0%> (-0.52%)	⬇️
src/Utilities/mockoptimizer.jl	94.62% <0%> (ø)	⬆️
src/Bridges/Bridges.jl	100% <0%> (ø)	⬆️
src/Bridges/functionize_bridge.jl	96.55% <0%> (ø)
... and 4 more

---

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 8b8b2ff...1aa0b7c. Read the comment docs.

[blegat]

For example, SCIP.jl excludes one test of the QCP suite, because it requires VectorAffineFunctions

It requires VectorAffineFunctions instead of what ?

[rschwarz]

It requires VectorAffineFunctions instead of what ?

The other QCP tests are written with ScalarAffineFunction (and ScalarQuadraticFunction).
So, a bridged SCIP solver could support them, I guess.

[blegat]

Yes, you could use the ScalarizeBridge

[rschwarz]

Sorry, this is kind of off-topic here, but let's clarify it anyways:

Yes, you could use the ScalarizeBridge

And we do: SCIP/test/MOI_wrapper.j.
I found that there is value in testing the directly supported features without any bridges.

Also, even using the ScalarizeBridge will not make these tests future-proof: Maybe somebody will add more tests that use quadratic objective functions which would not be bridged properly, so the new tests would fail in SCIP.jl.

[blegat]

I found that there is value in testing the directly supported features without any bridges.

Indeed, if you do that, you might be easily be broken by new tests. With the addition of Scalarize, Functionize, FlipSign and Slack bridges, solvers can have a very limited set of supported constraints and may not pass many tests without bridges. I would recommend running all tests with the optimizer bridged with full_bridge_optimizer as I just did in jump-dev/SCS.jl#142
Making a breaking release everytime we add a test wouldn't be practical.
If you want to run tests without bridges, I would suggest calling a specific test instead of running a test-set with an exclude list.

Maybe somebody will add more tests that use quadratic objective functions which would not be bridged properly, so the new tests would fail in SCIP.jl.

This should be resolved by #529 so it shouldn't weight in the long term decision on whether new tests mean breaking change :-P

[rschwarz]

If you want to run tests without bridges, I would suggest calling a specific test instead of running a test-set with an exclude list.

That's a good suggestion which I will implement now 👍

[blegat]

These two tests do not correspond to convex qcp for qcp4, it is quadratic-in-lessthan with a non-psd matrix and for qcp5 it is quadratic-in-equalto. We should probably make a different testset for non-convex or a sub-testset so that SOC solvers can easily exclude them all.

[rschwarz]

Fair enough, it was not obvious that the QCP testset should be limited to convex problems. Actually, the intent of these cases was not to test nonconvex constraints, but quadratic constraints with off-diagonal nonzeros, because they are treated differently w.r.t. to the scaling factor 2. So maybe it would be better to turn them into convex problems instead.

EDIT: or do both 😄. I just defined ncqcp which might not have an obvious meaning, but will also add another, convex test case.

[blegat]

Looks good, SCS and Mosek are passing these two new convex qcp tests