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

Broadcasted Operations Stall Julia #191

Closed
ChrisRackauckas opened this issue May 28, 2017 · 28 comments
Closed

Broadcasted Operations Stall Julia #191

ChrisRackauckas opened this issue May 28, 2017 · 28 comments

Comments

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented May 28, 2017

When broadcasting some operations, Julia will get stuck. Example:

using StaticArrays
u0 = @SMatrix rand(4,2)
reltol = 0.003
uprev = u0
u = u0
max.(abs.(u),abs.(uprev)) .* reltol

This is tested on the latest release (on v0.6-rc2) and on StaticArrays master.

@ChrisRackauckas ChrisRackauckas changed the title Broadcasted Operations Freeze Stall Julia Broadcasted Operations Stall Julia May 28, 2017
@c42f
Copy link
Member

c42f commented May 29, 2017

Yikes, I'm not sure what to make of this one yet. More minimal test case:

using StaticArrays
x = broadcast((a,b,c)->0, SVector(1,1), 0, 0)

@andyferris
Copy link
Member

Oh dear. These should be our implementation of broadcast, I suppose.

@bshall
Copy link
Contributor

bshall commented May 29, 2017

I think the issue is in the broadcast_sizes function. If you manually define the sizes @c42f example works

s = (Size{(2,)}(), Size(), Size())
StaticArrays._broadcast((a,b,c)->0, s, SVector(1,1), 0, 0)

But calling StaticArrays.broadcast_sizes(SVector(1,1), 0, 0) hangs.

@bshall
Copy link
Contributor

bshall commented May 29, 2017

It seems like this could be a julia bug. A test case is

f(t::Tuple) = t
f(t::Tuple, a::Int, as...) = f((t..., a), as...)
f(t::Tuple, a::Float64, as...) = f((t..., a), as...)

f((), 1, 1, 1) # works fine
f((), 1.0, 1.0, 1.0) # works fine
f((), 1, 1.0, 1) # hangs

What do you guys think @andyferris @c42f is this a bug or is there something wrong with the recursion? By the way, it doesn't even start the recursion, julia hangs after the initial call.

As a workaround for StaticArrays you don't really need to define broadcast_sizes recursively, you could just loop over the as to build up the sizes.

@c42f
Copy link
Member

c42f commented May 29, 2017

Yes, I was just coming to the same conclusion myself, I think this is a julia bug, at least in julia-0.6.0-rc2.

Unfortunately we can't use a simple loop, as we need this to be fully type inferred. We might be able to get away with a carefully constructed generated function...

@bshall
Copy link
Contributor

bshall commented May 29, 2017

I just checked on 0.5.1 and it also doesn't work there. Should I go ahead and report it?

Unfortunately we can't use a simple loop, as we need this to be fully type inferred

Oh right, that makes sense.

@andyferris
Copy link
Member

andyferris commented May 29, 2017

This is a bit odd. This is v0.6-rc2?

I know @vtjnash has been going on a mission lately to remove the "collector" pattern from Base because it doesn't interact with well with inference. See e.g. JuliaLang/julia#22019.

An example on how to do this pattern correctly is here:

https://github.com/JuliaLang/julia/pull/22019/files#diff-2264bb51acec4e7e2219a3cb1c733651R206

This pattern easily allows mapping, filtering, reducing and finding of tuple elements. It's important that types going to the inner functions are getting simpler (smaller tuples) not more complex (longer tuples). One "better" way to do our trivial f is:

f(a::Int, as...) = (a, f(as...)...)
f(a::Float64, as...) = (a, f(as...)...)

Note that the inner function has one less argument than the outer, so inference can feel safe that types are getting simpler. We should make StaticArrays adhere to such patterns, rather than the collector one (which I put everywhere).

@c42f
Copy link
Member

c42f commented May 29, 2017

Ok, nice that makes a lot of sense.

This is still a bug but the workaround seems easy.

@c42f
Copy link
Member

c42f commented May 29, 2017

PR coming.

@bshall
Copy link
Contributor

bshall commented May 29, 2017

I've got it working if you'd like me to do it?

@c42f
Copy link
Member

c42f commented May 29, 2017

Sure, push it up if you like. I've got it working locally but I was just refactoring to beef up the broadcast tests.

@andyferris
Copy link
Member

I like the competition. :)

@c42f
Copy link
Member

c42f commented May 29, 2017

Hah, I should be working on my logging package instead, given juliacon is nearly on us. @bshall has kindly agreed to finish it off :-)

@c42f
Copy link
Member

c42f commented May 30, 2017

Fixed in #196

@c42f c42f closed this as completed May 30, 2017
@ChrisRackauckas
Copy link
Member Author

Hey, is there a plan to tag StaticArrays anytime soon? I am wondering since this is making tests fail in DiffEq on v0.6, and I was hoping to get some updates in before JuliaCon.

@andyferris
Copy link
Member

Sure - why not?

@andyferris
Copy link
Member

@ChrisRackauckas
Copy link
Member Author

Thanks!

@andyferris
Copy link
Member

I haven't heard from @attobot yet...

@andyferris
Copy link
Member

Hmm... @JuliaArrays didn't have @attobot enabled for StaticArrays...

@andyferris
Copy link
Member

@simonbyrne how do I get attobot to pick up a release I've already made? And has github removed the ability to delete a release? The button shown in the attobot readme simply isn't there...

@KristofferC
Copy link
Contributor

Click on the release. Delete should be in the top right.

@andyferris
Copy link
Member

Thanks... I think I was in "edit". Silly me.

@andyferris
Copy link
Member

@chriselrod
Copy link
Contributor

More broadcast weirdness:

using StaticArrays
mv = MVector{50}(randn(50))
x = rand(50);
mv .= log.(x)
all(mv .== log(x[1]))

Returns (and is) true, while

all(mv .== log.(x))

Returns (and is) false.

@mschauer
Copy link
Collaborator

Which version are you using? Should this issue be reopened?

@chriselrod
Copy link
Contributor

chriselrod commented Jul 23, 2017

I tried it on both 0.6.1 and master, to the same results.

@c42f
Copy link
Member

c42f commented Jul 23, 2017

Please open a new issue for this.

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

No branches or pull requests

7 participants