Skip to content
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

tests failing on julia 0.6 #192

Closed
mlubin opened this issue Mar 19, 2017 · 7 comments
Closed

tests failing on julia 0.6 #192

mlubin opened this issue Mar 19, 2017 · 7 comments

Comments

@mlubin
Copy link
Member

mlubin commented Mar 19, 2017

In addition to deprecation warnings, I'm getting the following failure when running Convex.jl's tests on the latest julia 0.6 prerelease:

  > index atom
ERROR: LoadError: LoadError: LoadError: MethodError: no method matching getindex(::Convex.Variable, ::CartesianIndex{2})
Stacktrace:
 [1] collect(::Base.Generator{Base.LogicalIndex{CartesianIndex{2},Array{Bool,2}},Convex.##34#35{Convex.Variable}}) at ./array.jl:418
 [2] getindex(::Convex.Variable, ::Array{Bool,2}) at /home/mlubin/.julia/v0.6/Convex/src/atoms/affine/index.jl:88
 [3] (::##120#385)() at /home/mlubin/.julia/v0.6/Convex/test/test_affine.jl:136
 [4] context(::##120#385, ::String) at /home/mlubin/.julia/v0.6/FactCheck/src/FactCheck.jl:475
 [5] (::##37#302)() at /home/mlubin/.julia/v0.6/Convex/test/test_affine.jl:126
 [6] facts(::##37#302, ::String) at /home/mlubin/.julia/v0.6/FactCheck/src/FactCheck.jl:449
 [7] include_from_node1(::String) at ./loading.jl:539
 [8] include(::String) at ./sysimg.jl:14
 [9] macro expansion at /home/mlubin/.julia/v0.6/Convex/test/runtests_single_solver.jl:22 [inlined]
 [10] anonymous at ./<missing>:?
 [11] include_from_node1(::String) at ./loading.jl:539
 [12] include(::String) at ./sysimg.jl:14
 [13] macro expansion at /home/mlubin/.julia/v0.6/Convex/test/runtests.jl:36 [inlined]
 [14] anonymous at ./<missing>:?
 [15] include_from_node1(::String) at ./loading.jl:539
 [16] include(::String) at ./sysimg.jl:14
 [17] process_options(::Base.JLOptions) at ./client.jl:305
 [18] _start() at ./client.jl:371
while loading /home/mlubin/.julia/v0.6/Convex/test/test_affine.jl, in expression starting on line 6
while loading /home/mlubin/.julia/v0.6/Convex/test/runtests_single_solver.jl, in expression starting on line 20
while loading /home/mlubin/.julia/v0.6/Convex/test/runtests.jl, in expression starting on line 32
@madeleineudell
Copy link
Contributor

madeleineudell commented Mar 20, 2017 via email

@mlubin
Copy link
Member Author

mlubin commented Mar 20, 2017

@madeleineudell
Copy link
Contributor

Ok, here's an update I could use help on.

Broadcasting in Julia v0.6 is automatic; one no longer defines methods .*(x,y) but rather .*(x,y) automatically calls broadcast(::typeof(*), x, y).

However, for us that's a problem, since if x and y are both matrix variables, .*(x,y) = broadcast(::typeof(*), x, y) = *(x,y), since matrix variables do not appear to be Arrays to the compiler. And hence we get an error that x and y are not compatible sizes.

The simplest way to fix this problem would be to redefine broadcast, which I have so far been unable to do. See the branch ready-for-v0.6. I'd appreciate help here.

The nicest way (I think), which requires a significant rewrite of the code base, would be to make all matrix/vector variables into arrays of variables, so .* would automatically do the right thing. It's not clear how to do this without introducing significant overhead, since we'd have to check convexity / do conic form transformations elementwise.

@stevengj
Copy link

stevengj commented May 25, 2017

Yes, the right thing is to redefine broadcast when you have a new container type. See problem 2 in https://github.com/stevengj/18S096-iap17/blob/master/pset2/pset2-solutions.ipynb for a toy broadcast implementation if that is helpful.

@stevengj
Copy link

For a non-toy example that utilizes the full broadcast machinery, see: https://github.com/JuliaLang/julia/blob/master/base/sparse/higherorderfns.jl

(We are planning to make broadcast easier to extend in 1.0.)

@stevengj
Copy link

Oh, I see, life is more complicated for you because you need to guarantee convexity, so it is not possible for you to extend broadcast(f::Function, ...) for arbitrary f.

@madeleineudell
Copy link
Contributor

Closed by ba00f76. Unfortunately, it required a breaking change to the syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants