Skip to content
This repository has been archived by the owner on Dec 8, 2020. It is now read-only.

WIP: updates for Julia 1.0 #1

Merged
merged 75 commits into from
Oct 30, 2018
Merged

WIP: updates for Julia 1.0 #1

merged 75 commits into from
Oct 30, 2018

Conversation

molet
Copy link
Member

@molet molet commented Oct 23, 2018

Updating Convex.jl for Julia 1.0.

femtocleaner bot and others added 30 commits August 28, 2017 16:47
Fix minor typo in knapsack example
* updated struct and type parameter syntax to julia 0.6

* mutable structs, julia0.6
@@ -205,5 +205,5 @@ function broadcast(::typeof(*), x::AbstractExpr, y::AbstractExpr)
end
broadcast(::typeof(*), x::Value, y::AbstractExpr) = DotMultiplyAtom(Constant(x), y)
broadcast(::typeof(*), x::AbstractExpr, y::Value) = DotMultiplyAtom(Constant(y), x)
broadcast(::typeof(/), x::AbstractExpr, y::Value) = DotMultiplyAtom(Constant(1./y), x)
broadcast(::typeof(/), x::AbstractExpr, y::Value) = DotMultiplyAtom(Constant(1 ./ y), x)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe instead of overloading Base.broadcast directly, we're now supposed to go through a different set of machinery laid out in https://docs.julialang.org/en/v1/manual/interfaces/#man-interfaces-broadcasting-1. For now this is probably okay, since it does technically work to do it this way, but I can look into reworking this later.


vcat(args::AbstractExpr...) = transpose(HcatAtom(map(transpose, args)...))
vcat(args::AbstractExprOrValue...) = transpose(HcatAtom(map(arg -> transpose(convert(AbstractExpr, arg)), args)...))
vcat(args::Value...) = Base.cat(args..., dims=1) # Note: this makes general vcat slower for anyone using Convex...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this definition necessary? Value is defined as Union{Number, AbstractArray}, which should already work as expected with the vcat methods in Base.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I comment out this line I get some strange error messages for some tests. Very likely the problem is that in this case the vcat(args::AbstractExprOrValue...) function is called for arguments, which all have Value types but seemingly this function works only for arguments that are either all AbstractExpr types or mixed AbstractExpr and Value types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, it's because the fallback method that vcat([3], 3), for example, uses is untyped. This is good as is except it should use Val like the Base definition:

vcat(X...) = cat(X...; dims=Val(1))

Base.vect(args::AbstractExpr...) = transpose(HcatAtom(map(transpose, args)...))
Base.vect(args::AbstractExprOrValue...) = transpose(HcatAtom(map(arg -> transpose(convert(AbstractExpr, arg)), args)...))

function Base.vect(args::Value...)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This similarly seems unnecessary, as the Base methods work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing like above.

src/atoms/affine/index.jl Outdated Show resolved Hide resolved
src/atoms/affine/index.jl Outdated Show resolved Hide resolved
x = Variable(10)
a = rand(10, 1)
a = shuffle(collect(0.1:0.1:1.0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the switch to shuffle here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright.

You should probably change it to 0.0:0.1:0.9 though as that is within the range of rand while 1.0 is not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these tests the actual numbers don't really matter and they even don't need to be strictly within the range [0, 1). If possible I would rather not touch them as they finally seem to be passing nicely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright sure

Copy link
Member

@iamed2 iamed2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I think this is good enough for a first merge, then we can make a WIP PR to Convex.jl to announce our intent and keep working on the harder bits like broadcast.

@iamed2 iamed2 merged commit 11025c2 into master Oct 30, 2018
@ararslan ararslan deleted the lm/v1.0 branch October 30, 2018 18:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants