Update to support Julia 1.0 #239

iamed2 · 2018-10-30T18:19:42Z

We've been working on this in a fork and will contribute it back.

~~We have yet to tackle broadcast or add in any new linalg idioms from 1.0.~~

Fixes #235
Fixes #231

ararslan · 2018-10-30T18:37:22Z

It would be good if we could clean up the commits here. Letif's PR into Invenia's master probably could have been squashed merged.

little changes + ECOS is now the first tested Solver norm is imported norm is restored starting fixing tests undefined arrays fixed testing diag function import Base./ iterate implemented + import functions from LinearAlgebra many tests are fixed up to test_sdp.jl fix diagm diagm fixed in tests +(::Array{Float64,2}, ::Float64) fixed +(x::Array{Float64,2}, y::Number) implemented diagm is fixed in sdp_constraints.jl fix norm function assert is fixed in power_to_socp.jl all assert issues fixed in power_to_socp.jl cat function fixed in stack.jl test_socp.jl fixed test_params.jl fixed fixes based on review explicit solver is added to Problem function order of arguments changed in Problem function evaluate(x::AdditionAtom) fixed, starting rewriting tests evaluate(x::AdditionAtom) fixed satisfy function is fixed ArgumentError implemented to diagm function fix! function is fixed fix! fixed again diagandlowerpart in conic_form! function is fixed hvcat implemented for types AbstractExpr & AbstractExprOrValue hvcat fixed hvcat exported solver argument removed from Problem function type Problem fixed argument order in function Problem restored eye functions fixed trace => tr fixed trace => tr fixed in tests norm => opnorm fixed in operatornorm.jl fixing tests finished replacing Diagonal reduce redundancy of hvcat stack.jl cleaned fix test failures due to random seed in test_lp.jl reducing conv threshold of SCSSolver to 1e-12 reducing conv threshold of SCSSolver to 1e-12 skip problematic max/min atom tests for SCS solver several changes based on second round of review ctranspose => adjoint & CTransposeAtom => AdjointAtom comment out hvcat uncomment hvcat trying to fix tests for SCS solver tests for SCS solver fixed comment in add_subtract.jl added + deprecated.jl removed hcat/vcat/vect with Value arguments commented out hcat uncommented uncomment vcat uncomment vect test vect test vcat stack.jl cleaned dims=1 => dims=Val(1) for vcat and dim=2 => dims=Val(2) for hcat with Value arguments changes in index.jl & fixed mutable struct in power_to_socp.jl

iamed2 · 2018-10-30T19:18:36Z

All squashed now

It was upper bounded at 0.4.0 for Julia 0.6 support.

ararslan · 2018-10-30T19:28:40Z

FYI I'll be continuing to make updates as PRs against Invenia's fork. See for example invenia#2.

The package no longer supports Julia 0.6 or earlier.

This is consistent with how one otherwise uses diagm.

More updates for Julia 1.0

ararslan · 2018-10-31T18:32:00Z

Dropping the WIP label as I think this should now be ready for review.

REQUIRE

mlubin

LGTM superficially, besides the MPB upper bound. I haven't done a through review.

chriscoey · 2018-10-31T19:08:37Z

2-space and 4-space indenting seem to be used inconsistently. FWIW I prefer consistent 4-spacing.

iamed2 · 2018-10-31T19:12:28Z

2-space and 4-space indenting seem to be used inconsistently. FWIW I prefer consistent 4-spacing.

That's an artifact of the existing code. Maybe we can save that for a separate PR after?

ararslan · 2018-10-31T19:32:28Z

I'm also bothered by the inconsistent indentation in this repo, but I agree with Eric, that would be better left for a separate PR. For the changes I made, I just tried to follow the style used in the surrounding code.

mlubin · 2018-11-01T02:12:20Z

Is the reported 17% drop in code coverage real or a fluke?

.travis.yml

ararslan · 2018-11-01T04:41:11Z

The coverage drop is JuliaCI/Coverage.jl#187. I wouldn't expect these changes to impact coverage in any meaningful way.

matbesancon · 2018-11-01T14:14:27Z

src/atoms/second_order_cone/power_to_socp.jl

@@ -12,7 +12,6 @@
 # This reduction is documented in the pdf available at
 # https://github.com/JuliaOpt/Convex.jl/raw/master/docs/supplementary/rational_to_socp.pdf

-using Compat
 module psocp


module name psocp => Psocp or PSCOP maybe?

src/atoms/second_order_cone/power_to_socp.jl

matbesancon · 2018-11-01T14:23:21Z

src/variable.jl

-  # TODO: check sizes match
-  x.value = Array{Float64}(size(x))
-  x.value[:] = v
+function fix!(x::Variable, v::AbstractArray)


maybe a newline between end of function and new one, & docstring for fix!

matbesancon · 2018-11-01T14:26:56Z

test/runtests.jl


 # Seed random number stream to improve test reliability
-srand(2)
+Random.seed!(2)

 solvers = Any[]


const solvers = Any[ECOSSolver(verbose=0), GLPKSolverMIP(), SCSSolver(verbose=0, eps=1e-6)]
(push! still works)

matbesancon · 2018-11-01T14:30:13Z

src/atoms/affine/transpose.jl

  return evaluate(x.children[1])'
 end

 # Since everything is vectorized, we simply need to multiply x by a permutation
 # matrix such that coeff * vectorized(x) - vectorized(x') = 0
-function conic_form!(x::CTransposeAtom, unique_conic_forms::UniqueConicForms=UniqueConicForms())
+function conic_form!(x::AdjointAtom, unique_conic_forms::UniqueConicForms=UniqueConicForms())


The comment above conic_form! could be a docstring

matbesancon · 2018-11-01T14:30:42Z

src/atoms/exp_cone/entropy.jl

@@ -13,7 +13,7 @@ export sign, curvature, monotonicity, evaluate
 # TODO: make this work for a *list* of inputs, rather than just for scalar/vector/matrix inputs

 # Entropy atom: -xlogx entrywise
-type EntropyAtom <: AbstractExpr
+mutable struct EntropyAtom <: AbstractExpr


the comment above should be a docstring

matbesancon · 2018-11-01T14:31:22Z

src/atoms/sdp_cone/nuclearnorm.jl

@@ -40,7 +40,7 @@ end
 nuclearnorm(x::AbstractExpr) = NuclearNormAtom(x)

 # Create the equivalent conic problem:
-#   minimize (trace(U) + trace(V))/2
+#   minimize (tr(U) + tr(V))/2


the whole comment block should be the docstring

ararslan · 2018-11-01T17:14:37Z

@matbesancon Thanks for the review! Adding docstrings and renaming modules is out of the intended scope of this PR but would be good future work. I'd suggest opening an issue to track that. Basically we're trying to make the minimum possible set of changes for Julia 1.0 support right now.

This is in keeping with general JuliaOpt practices.

It was changed amongst the other 1.0 compatibility changes but does not seem to be necessary.

matbesancon · 2018-11-01T21:16:12Z

you're right, I'll wait for this one to get merge and will probably make one

Fixes jump-dev#241

This fixes the case where an `AbstractExpr` occursin in a fused broadcast expresion.

madeleineudell

This update looks great! I've reviewed the changes line by line, and can confirm that they update syntax but don't change the meaning. Thanks for all your work getting the syntax up to date!

madeleineudell · 2018-11-02T21:40:15Z

src/atoms/affine/partialtrace.jl

@@ -87,8 +87,8 @@ function conic_form!(x::PartialTraceAtom, unique_conic_forms::UniqueConicForms=U
        # in the system we want to trace out
        # This function returns every term in the sum
        function term(ρ, j::Int)
-            a = speye(1)
-            b = speye(1)
+            a = sparse(1.0I, 1, 1)


This is a pretty awkward way to form a 1x1 sparse identity matrix. But that's Julia 1.0, I guess...

There's also spdiagm(0 => [1.0]) but yeah it's unfortunate.

For dense arrays there's Eye from https://github.com/JuliaArrays/FillArrays.jl

madeleineudell · 2018-11-02T21:46:14Z

src/atoms/sdp_cone/lambda_min_max.jl

@@ -107,7 +107,7 @@ function conic_form!(x::LambdaMinAtom, unique_conic_forms)
    A = x.children[1]
    m, n = size(A)
    t = Variable()
-    p = maximize(t, A - t*eye(n) ⪰ 0)
+    p = maximize(t, A - t*Matrix(1.0I, n, n) ⪰ 0)


A - t * I would work here. (No need to specify dimensions, since they're implied by size of A.)

Good call, I've implemented this (as well as your other such suggestions) in invenia#7.

Seems this will have to do as-is; we don't have machinery in place to handle UniformScaling. That could be added in the future though.

madeleineudell · 2018-11-02T21:47:31Z

src/atoms/sdp_cone/operatornorm.jl

@@ -53,7 +53,7 @@ function conic_form!(x::OperatorNormAtom, unique_conic_forms)
    A = x.children[1]
    m, n = size(A)
    t = Variable()
-    p = minimize(t, [t*speye(m) A; A' t*speye(n)] ⪰ 0)
+    p = minimize(t, [t*sparse(1.0I, m, m) A; A' t*sparse(1.0I, n, n)] ⪰ 0)


t*I would work here. (No need to specify dimensions.)

madeleineudell · 2018-11-02T21:47:52Z

src/atoms/sdp_cone/sumlargesteigs.jl

-    p = minimize(s*k + trace(Z), 
-                 Z + s*eye(n) - A ⪰ 0, 
+    p = minimize(s*k + tr(Z), 
+                 Z + s*Matrix(1.0I, n, n) - A ⪰ 0,


s*I would work here. (No need to specify dimensions.)

madeleineudell · 2018-11-02T22:06:46Z

test/test_lp.jl

  end

  @testset "max atom" begin
    x = Variable(10, 10)
    y = Variable(10, 10)
-    a = rand(10, 10)
-    b = rand(10, 10)
+    a = reshape(shuffle(collect(0.01:0.01:1.0)), (10, 10))


(Not that it matters, but) why this change?

See invenia#1 (comment)

This fixed an issue that was tricky to track down. @molet noted in invenia#1 (comment):

Using rand led to test failures with SCS solver for some cases. I think it happened because for some set of random numbers the problem might not be well conditioned (i.e. some numbers are too close to each other). So I changed it to a set of numbers from an equidistant grid but to still put some randomness there I shuffled them.

Overload broadcasted instead of broadcast

Don't overwrite Base.vect for numbers with a different implementation

ararslan · 2018-11-05T19:30:04Z

Things seem to be in good shape, so I'd like to get this merged soon. Thanks to everyone who has reviewed!

iamed2 force-pushed the master branch from 9c888bb to 3138f31 Compare October 30, 2018 19:18

Set the SCS lower bound for testing to 0.4.0

e311885

It was upper bounded at 0.4.0 for Julia 0.6 support.

ararslan and others added 6 commits October 30, 2018 12:30

srand is now Random.seed!

45b8f12

Remove unused dependency on Compat

7dcf8ff

The package no longer supports Julia 0.6 or earlier.

Extend diagm for AbstractExpr using the Pair interface

fe5b46d

This is consistent with how one otherwise uses diagm.

Load kron from LinearAlgebra instead of Base

f317d50

Update coverage submission step for Pkg3

fc82a2c

Merge pull request #2 from invenia/aa/more-1.0

8f0696b

More updates for Julia 1.0

ararslan changed the title ~~WIP: Update to support Julia 1.0~~ Update to support Julia 1.0 Oct 31, 2018

mlubin reviewed Oct 31, 2018

View reviewed changes

REQUIRE Outdated Show resolved Hide resolved

mlubin approved these changes Oct 31, 2018

View reviewed changes

ararslan mentioned this pull request Oct 31, 2018

Reinstate upper bound on MathProgBase at 0.8 invenia/Convex.jl#3

Merged

Reinstate upper bound on MathProgBase at 0.8 (#3)

5a033f7

omus mentioned this pull request Oct 31, 2018

Inconsistent use of whitespace #240

Closed

mlubin reviewed Nov 1, 2018

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

matbesancon reviewed Nov 1, 2018

View reviewed changes

ararslan mentioned this pull request Nov 1, 2018

Updates based on upstream PR review invenia/Convex.jl#4

Merged

ararslan added 2 commits November 1, 2018 11:24

Remove testing on Julia nightly

530feed

This is in keeping with general JuliaOpt practices.

Remove introduced mutability of SimpleInequalityExpression

f111790

It was changed amongst the other 1.0 compatibility changes but does not seem to be necessary.

This was referenced Nov 1, 2018

Unable to use Convex.jl in Julia v1.0 #235

Closed

norm(matrix) -> opnorm(matrix) in Julia 0.7 #231

Closed

ararslan added 2 commits November 2, 2018 13:26

Overwrite Base.vect for numbers with the same implementation as Base

c851ac9

Fixes jump-dev#241

Overload broadcasted instead of broadcast

b3396fb

This fixes the case where an `AbstractExpr` occursin in a fused broadcast expresion.

ararslan mentioned this pull request Nov 2, 2018

Convex.jl changes behaviour of vector construction #241

Closed

madeleineudell approved these changes Nov 2, 2018

View reviewed changes

ararslan mentioned this pull request Nov 2, 2018

Use I in more cases instead of materializing matrices invenia/Convex.jl#7

Closed

iamed2 added 2 commits November 2, 2018 17:30

Merge pull request #6 from invenia/aa/broadcasted

9fb4107

Overload broadcasted instead of broadcast

Merge pull request #5 from invenia/aa/fix-vect

0018ab9

Don't overwrite Base.vect for numbers with a different implementation

ararslan merged commit 4152844 into jump-dev:master Nov 5, 2018

ericphanson mentioned this pull request Jun 28, 2019

Too tight tolerance in one of the tests? #295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to support Julia 1.0 #239

Update to support Julia 1.0 #239

iamed2 commented Oct 30, 2018 •

edited by ararslan

Loading

ararslan commented Oct 30, 2018

iamed2 commented Oct 30, 2018

ararslan commented Oct 30, 2018

ararslan commented Oct 31, 2018

mlubin left a comment

chriscoey commented Oct 31, 2018

iamed2 commented Oct 31, 2018

ararslan commented Oct 31, 2018

mlubin commented Nov 1, 2018

ararslan commented Nov 1, 2018

matbesancon Nov 1, 2018

matbesancon Nov 1, 2018

matbesancon Nov 1, 2018

matbesancon Nov 1, 2018

matbesancon Nov 1, 2018

matbesancon Nov 1, 2018

ararslan commented Nov 1, 2018

matbesancon commented Nov 1, 2018

madeleineudell left a comment

madeleineudell Nov 2, 2018

iamed2 Nov 2, 2018

madeleineudell Nov 2, 2018

ararslan Nov 2, 2018

ararslan Nov 3, 2018

madeleineudell Nov 2, 2018

madeleineudell Nov 2, 2018

madeleineudell Nov 2, 2018

iamed2 Nov 2, 2018

ararslan Nov 2, 2018

ararslan commented Nov 5, 2018

Update to support Julia 1.0 #239

Update to support Julia 1.0 #239

Conversation

iamed2 commented Oct 30, 2018 • edited by ararslan Loading

ararslan commented Oct 30, 2018

iamed2 commented Oct 30, 2018

ararslan commented Oct 30, 2018

ararslan commented Oct 31, 2018

mlubin left a comment

Choose a reason for hiding this comment

chriscoey commented Oct 31, 2018

iamed2 commented Oct 31, 2018

ararslan commented Oct 31, 2018

mlubin commented Nov 1, 2018

ararslan commented Nov 1, 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

ararslan commented Nov 1, 2018

matbesancon commented Nov 1, 2018

madeleineudell 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

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

ararslan commented Nov 5, 2018

iamed2 commented Oct 30, 2018 •

edited by ararslan

Loading