Enh/options #110

kalmarek · 2018-09-26T01:58:11Z

Would be great if someone could test it on the MOI side;

I took a simple route of extracting the Direct/Indirect type from options in optimize! (as it was done in MPB), but that could be easily changed to Optimizer (and SCSMathProgModel) parametrized by the type

Edit by mlubin@: Closes #94

since scs-2.* these tests weren't testing anything as solver obtained accuracy of 1e-5 without effort

blegat · 2018-09-26T08:24:10Z

I tried the tests with the Direct option:

diff --git a/test/MOIWrapper.jl b/test/MOIWrapper.jl
index 84aa941..2999cd1 100644
--- a/test/MOIWrapper.jl
+++ b/test/MOIWrapper.jl
@@ -14,7 +14,7 @@ MOIU.@model(SCSModelData,
             (MOI.ScalarAffineFunction,),
             (MOI.VectorOfVariables,),
             (MOI.VectorAffineFunction,))
-const optimizer = MOIU.CachingOptimizer(SCSModelData{Float64}(), SCS.Optimizer())
+const optimizer = MOIU.CachingOptimizer(SCSModelData{Float64}(), SCS.Optimizer(linear_solver=SCS.Direct))
 
 # linear9test needs 1e-3 with SCS < 2.0 and 5e-1 with SCS 2.0
 # linear2test needs 1e-4

and it passes all the tests while showing Lin-sys: sparse-direct in the log

kalmarek · 2018-09-26T22:14:08Z

btw. if tests fail on SCS-2.* because of instability You may always set alpha=1.9 and acceleration_lookback=1 to bring back the old behaviour

kalmarek · 2018-09-26T22:16:25Z

The whole code would be much cleaner if SCS(set_default_settings) wouldn't require the whole SCSData, but operate directly on SCSSettings. Any particular reason for this choice @bodono ?

mlubin

Thanks for making this update. Some comments below.

mlubin · 2018-09-26T22:29:41Z

src/MOIWrapper.jl

@@ -282,7 +284,15 @@ function MOI.optimize!(optimizer::Optimizer)
    objconstant = optimizer.data.objconstant
    c = optimizer.data.c
    optimizer.data = nothing # Allows GC to free optimizer.data before A is loaded to SCS
-    sol = SCS_solve(SCS.Indirect, m, n, A, b, c, cone.f, cone.l, cone.qa, cone.sa, cone.ep, cone.ed, cone.p)
+
+    T = SCS.Indirect # the default method


Nit: Please use a more descriptive variable name than T. Maybe linear_solver_method or just linear_solver.

mlubin · 2018-09-26T22:30:31Z

src/c_wrapper.jl

+    end
+
+    for param in keys(opts)
+        # when dropping support for 0.6 could be replaced with setproperty!


What could be replaced exactly? To make sure this happens, you could use a VERSION if/else.

mlubin · 2018-09-26T22:34:07Z

src/types.jl

@@ -37,7 +37,7 @@ SCSMatrix(m::ManagedSCSMatrix) =
    SCSMatrix(pointer(m.values), pointer(m.rowval), pointer(m.colptr), m.m, m.n)


-struct SCSSettings
+mutable struct SCSSettings


I don't think this change is valid. SCSSettings needs to mirror the C struct in order for the ccalls to be correct. mutable structs can't be used to mirror C structs AFAIK.

mlubin · 2018-09-26T22:34:29Z

test/MOIWrapper.jl

-# linear9test needs 1e-3 with SCS < 2.0 and 5e-1 with SCS 2.0
-# linear2test needs 1e-4
-const config = MOIT.TestConfig(atol=1e-4, rtol=1e-4)
+    # linear9test needs 1e-3 with SCS < 2.0 and 5e-1 with SCS 2.0


Why do we care about tolerances for an SCS version that we don't use anymore? (Not needed in the comment.)

The tolerance needed for SCS < 2.0 is mentionned to show that this is a regression

It's unlikely that this code will handle SCS < 2.0 in the future, so this isn't the right place to discuss regressions.

mlubin · 2018-09-26T22:35:53Z

src/types.jl

@@ -37,7 +37,7 @@ SCSMatrix(m::ManagedSCSMatrix) =
    SCSMatrix(pointer(m.values), pointer(m.rowval), pointer(m.colptr), m.m, m.n)


-struct SCSSettings
+mutable struct SCSSettings
    normalize::Int # boolean, heuristic data rescaling: 1


Please update these comments to reflect that the default parameters are set by SCS_set_default_settings! (i.e., remove the values because we have no way to make sure they are correct).

mlubin · 2018-09-26T22:37:34Z

src/c_wrapper.jl

@@ -86,6 +100,11 @@ end
 # Take Ref{}s because SCS might modify the structs
 for (T, lib) in zip([SCS.Direct, SCS.Indirect], [SCS.direct, SCS.indirect])
    @eval begin
+
+        function SCS_set_default_settings!(::Type{$T}, data::Ref{SCSData})


Naming nit: The name implies modification, so JuMP style recommends dropping the ! from the name (http://www.juliaopt.org/JuMP.jl/latest/style.html#Use-of-!-1). This is also consistent with SCS_init, SCS_solve, and SCS_finish.

mlubin · 2018-09-26T22:38:10Z

test/MOIWrapper.jl

-end
+    @testset "Continuous linear problems" begin
+        # AlmostSuccess for linear9 with SCS 2
+        MOIT.contlineartest(MOIB.SplitInterval{Float64}(optimizer), config, ["linear9"])


Is there a reasonable parameter setting that gets linear9 passing?

We need to set tolerance to 5e-1 last time I tested but it seems a bit restrictive to set it for all tests.
linear9test(optimizer, MOIT.TestConfig(atol=0.5, rtol=0.5) could be run separately in the line below

SCS.Optimizer(linear_solver=T, eps=1e-8)) config = MOIT.TestConfig(atol=1e-5)

passes MOIT.contlineartest(MOIB.SplitInterval{Float64}(optimizer), config) with both Direct and Indirect solver;

btw. You need to add some tests for passing options to SCS.Optimizer. only playing with this allowed me to catch missing ; in the argument list.

I think I just ran into the same issue where SCS.Optimizer does not accept solver options.

the intermediate _SCS_user_settings function serves as a check that user supplied only recognized options

kalmarek · 2018-09-27T13:01:42Z

if user supplies a wrong kwarg it will result in

SCSSettings(SCS.Indirect, eps=1e-12, epps=0.1)
ERROR: MethodError: no method matching _SCS_user_settings(::SCS.SCSSettings; epps=0.1, eps=1.0e-12)
Closest candidates are:
  _SCS_user_settings(::Any; normalize, scale, rho_x, max_iters, eps, alpha, cg_rate, verbose, warm_start, acceleration_lookback) at /home/kalmar/.julia/v0.6/SCS/src/types.jl:70 got unsupported keyword argument "epps"
....

I think it is informative enough

mlubin · 2018-09-27T13:17:26Z

I think it is informative enough

Not really, see the style discussion here. Please at least add a TODO to improve the error message.

bodono · 2018-09-27T13:52:47Z

The whole code would be much cleaner if SCS(set_default_settings) wouldn't require the whole SCSData, but operate directly on SCSSettings. Any particular reason for this choice @bodono ?

I don't think there is a particularly good reason for that choice, and it should probably just take the settings struct, but changing it now would be breaking.

kalmarek · 2018-09-27T16:10:22Z

I think it is informative enough

Not really, see the style discussion here. Please at least add a TODO to improve the error message.

is try ... catch MethodError error("You probably supplied unrecognized option to SCS.Optimizer ") acceptable?

kalmarek · 2018-09-27T16:16:41Z

@bodono It'd be great if it could be changed the next release, but it's definitely not critical

mlubin · 2018-09-27T16:44:55Z

is try ... catch MethodError error("You probably supplied unrecognized option to SCS.Optimizer ") acceptable?

You can't catch specific error types. You have to catch all errors, check the type, and rethrow if it's not what you expected.
This is not convincingly better than the MethodError you showed because it hides the name of the incorrect option.

kalmarek · 2018-09-28T00:04:18Z

julia> SCSSettings(SCS.Indirect, epps=0.1, e=0.3, eps=1e-12)
ERROR: ArgumentError: Unrecognized options passed to the SCSSolver: epps, e
Stacktrace:
....

mlubin · 2018-09-28T00:06:53Z

src/types.jl

+    catch err
+        if err isa MethodError
+            SCS_kwargs = fieldnames(default_settings[])
+            kwargs = [err.args[1][i*2-1] for i in 1:(length(err.args[1]) ÷ 2)]


There are a number of ways that this code could stop working unintentionally, so please add tests. The structure of err could be changed across Julia versions or _SCS_user_settings could be changed.

I can @test_throws ArgumentError ... against misspelled options, but that's it.
_SCS_user_settings is an internal function, it won't change beyond our control (btw. why is SCSSettings exported? I had always assumed that user is not supposed to construct it?)

I'd use the standard MethodError which actually tells exactly which options were not recognized, but that clashes with the style guide. What are Your suggestions on handling this?

I can @test_throws ArgumentError ... against misspelled options, but that's it.

That would make good tests :)

it won't change beyond our control

yes but when we change it we might forget to change MOIWrapper.jl.

add test to check the behaviour

mlubin · 2018-09-28T14:02:18Z

test/options.jl

+m = MathProgBase.ConicModel(s)
+MathProgBase.loadproblem!(m, -obj, A, rowub, [(:NonNeg,1:3)],[(:NonNeg,1:5)])
+
+@test_throws ArgumentError MathProgBase.optimize!(m)


Is there some way to test the text of the error message? That's pretty important in this case.

https://github.com/JuliaLang/julia/blob/e7d15d4a013a43442b75ba4e477382804fa4ac49/stdlib/Test/src/Test.jl#L532
@test_throws wants type, not an instance

Yes, @test_throws is probably not the way to do it. It may need a try/catch where you test the error message inside the catch and test that the catch block executed.

ccoffrin · 2018-09-28T23:31:57Z

Just as a general comment. There were some fairly big changes in the algorithm's performance since the last version. The change in the default behavior led my CI to hang indefinitely while waiting for SCS to converge.

In the end adding the parameter acceleration_lookback=1 resolved the issue. However, I don't think I would have been able to figure this out without Miles' assistance.

kalmarek · 2018-09-29T18:03:42Z

@ccoffrin its a known issue: cvxgrp/scs#93
the new (unreleased) version 2.1.0 in https://github.com/cvxgrp/scs/tree/2.1.0 tries to use L1 cost which alleviates the problem, but not fully;

alpha=1.9, acceleration_lookback=1 reverses SCS to pre-2.0 behaviour

update the attained precision add "eps=1e-7" to SCSSolver which tests MPBWrapper codepaths responsible for cleaning options

linear9 passes

kalmarek · 2018-10-08T08:57:32Z

I separated the option checking into sanatize_SCS_options function, there is no parsing of exceptions anymore (which was tad fragile);

please have a look again

kalmarek added 6 commits September 26, 2018 03:04

replace linearsolver by linear_solver

fa20604

use the SCS(set_default_settings)

7cc5565

check that all the supplied options are correct

a1476b1

modify SCSSettings from options provided in kwargs

43c8879

add options to MOI

68ff2f4

fix options tests

76bcf65

since scs-2.* these tests weren't testing anything as solver obtained accuracy of 1e-5 without effort

repeat MOI tests for both Direct and Indirect solvers

5e4d95c

mlubin reviewed Sep 26, 2018

View reviewed changes

kalmarek added 4 commits September 27, 2018 14:14

remove the default values from comments in SCSSettings

7d3d1ce

external constructor for SCSSettings with options

16e3f9d

the intermediate _SCS_user_settings function serves as a check that user supplied only recognized options

remove ! from SCS_set_default_settings

af3609e

replace T by linear_solver

86a8bb2

another try at throwing errors

6e2d775

mlubin reviewed Sep 28, 2018

View reviewed changes

throw ArgumentError in SCSSettings only on unrecognized kwords

e2b1e70

add test to check the behaviour

mlubin reviewed Sep 28, 2018

View reviewed changes

kalmarek added 2 commits September 29, 2018 23:10

test MPBWrapper with both Direct and Indirect linear_solvers

c93f64b

update the attained precision add "eps=1e-7" to SCSSolver which tests MPBWrapper codepaths responsible for cleaning options

test MOIWrapper with "eps=1e-8"

d6c68e6

linear9 passes

kalmarek added 5 commits September 29, 2018 23:14

test for precise ArgumentError message

78b6b63

ignore build.log

3702e60

perform all the checks in optimize! using sanatize_SCS_options

5f422d7

tests for ArgumentError strings

db60fb9

remove checking options via try catch

ab9ead7

blegat mentioned this pull request Sep 30, 2018

Solver Parameters in MOI #113

Closed

Merge branch 'master' into enh/options

7c199f8

blegat approved these changes Oct 13, 2018

View reviewed changes

mlubin merged commit 85aaf92 into jump-dev:master Oct 14, 2018

mlubin mentioned this pull request Oct 14, 2018

Direct mode isn't exposed in the MOI wrapper #108

Closed

ccoffrin mentioned this pull request Oct 22, 2018

Update Julia v0.6 for SCS v0.4 jump-dev/Pajarito.jl#419

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enh/options #110

Enh/options #110

kalmarek commented Sep 26, 2018 •

edited by mlubin

Loading

blegat commented Sep 26, 2018

kalmarek commented Sep 26, 2018

kalmarek commented Sep 26, 2018

mlubin left a comment

mlubin Sep 26, 2018

mlubin Sep 26, 2018

mlubin Sep 26, 2018

mlubin Sep 26, 2018 •

edited

Loading

blegat Sep 27, 2018

mlubin Sep 27, 2018

mlubin Sep 26, 2018

mlubin Sep 26, 2018

mlubin Sep 26, 2018

blegat Sep 27, 2018

kalmarek Sep 29, 2018

ccoffrin Sep 29, 2018

kalmarek commented Sep 27, 2018

mlubin commented Sep 27, 2018

bodono commented Sep 27, 2018

kalmarek commented Sep 27, 2018

kalmarek commented Sep 27, 2018

mlubin commented Sep 27, 2018

kalmarek commented Sep 28, 2018

mlubin Sep 28, 2018

kalmarek Sep 28, 2018

blegat Sep 28, 2018

mlubin Sep 28, 2018

kalmarek Sep 28, 2018

mlubin Sep 28, 2018

ccoffrin commented Sep 28, 2018 •

edited

Loading

kalmarek commented Sep 29, 2018

kalmarek commented Oct 8, 2018

Enh/options #110

Enh/options #110

Conversation

kalmarek commented Sep 26, 2018 • edited by mlubin Loading

blegat commented Sep 26, 2018

kalmarek commented Sep 26, 2018

kalmarek commented Sep 26, 2018

mlubin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlubin Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalmarek commented Sep 27, 2018

mlubin commented Sep 27, 2018

bodono commented Sep 27, 2018

kalmarek commented Sep 27, 2018

kalmarek commented Sep 27, 2018

mlubin commented Sep 27, 2018

kalmarek commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoffrin commented Sep 28, 2018 • edited Loading

kalmarek commented Sep 29, 2018

kalmarek commented Oct 8, 2018

kalmarek commented Sep 26, 2018 •

edited by mlubin

Loading

mlubin Sep 26, 2018 •

edited

Loading

ccoffrin commented Sep 28, 2018 •

edited

Loading