-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to support Julia 1.0 #239
Conversation
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
All squashed now |
It was upper bounded at 0.4.0 for Julia 0.6 support.
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
Dropping the WIP label as I think this should now be ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM superficially, besides the MPB upper bound. I haven't done a through review.
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? |
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. |
Is the reported 17% drop in code coverage real or a fluke? |
The coverage drop is JuliaCI/Coverage.jl#187. I wouldn't expect these changes to impact coverage in any meaningful way. |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module name psocp
=> Psocp
or PSCOP
maybe?
# TODO: check sizes match | ||
x.value = Array{Float64}(size(x)) | ||
x.value[:] = v | ||
function fix!(x::Variable, v::AbstractArray) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a newline between end of function and new one, & docstring for fix!
|
||
# Seed random number stream to improve test reliability | ||
srand(2) | ||
Random.seed!(2) | ||
|
||
solvers = Any[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const solvers = Any[ECOSSolver(verbose=0), GLPKSolverMIP(), SCSSolver(verbose=0, eps=1e-6)]
(push!
still works)
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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above conic_form!
could be a docstring
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment above should be a docstring
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the whole comment block should be the docstring
@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.
you're right, I'll wait for this one to get merge and will probably make one |
This fixes the case where an `AbstractExpr` occursin in a fused broadcast expresion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty awkward way to form a 1x1 sparse identity matrix. But that's Julia 1.0, I guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A - t * I
would work here. (No need to specify dimensions, since they're implied by size of A
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I've implemented this (as well as your other such suggestions) in invenia#7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t*I
would work here. (No need to specify dimensions.)
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s*I
would work here. (No need to specify dimensions.)
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not that it matters, but) why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Things seem to be in good shape, so I'd like to get this merged soon. Thanks to everyone who has reviewed! |
We've been working on this in a fork and will contribute it back.
We have yet to tacklebroadcast
or add in any new linalg idioms from 1.0.Fixes #235
Fixes #231