-
Notifications
You must be signed in to change notification settings - Fork 152
Broadcasted Operations Stall Julia #191
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
Comments
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) |
Oh dear. These should be our implementation of broadcast, I suppose. |
I think the issue is in the s = (Size{(2,)}(), Size(), Size())
StaticArrays._broadcast((a,b,c)->0, s, SVector(1,1), 0, 0) But calling |
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 |
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... |
I just checked on 0.5.1 and it also doesn't work there. Should I go ahead and report it?
Oh right, that makes sense. |
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 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(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). |
Ok, nice that makes a lot of sense. This is still a bug but the workaround seems easy. |
PR coming. |
I've got it working if you'd like me to do it? |
Sure, push it up if you like. I've got it working locally but I was just refactoring to beef up the broadcast tests. |
I like the competition. :) |
Hah, I should be working on my logging package instead, given juliacon is nearly on us. @bshall has kindly agreed to finish it off :-) |
Fixed in #196 |
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. |
Sure - why not? |
Thanks! |
I haven't heard from @attobot yet... |
Hmm... @JuliaArrays didn't have @attobot enabled for StaticArrays... |
@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... |
Click on the release. Delete should be in the top right. |
Thanks... I think I was in "edit". Silly me. |
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. |
Which version are you using? Should this issue be reopened? |
I tried it on both 0.6.1 and master, to the same results. |
Please open a new issue for this. |
When broadcasting some operations, Julia will get stuck. Example:
This is tested on the latest release (on v0.6-rc2) and on StaticArrays master.
The text was updated successfully, but these errors were encountered: