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

Incorrect element type with .^ after importing Gadfly #78

Closed
rdeits opened this issue Nov 14, 2016 · 19 comments
Closed

Incorrect element type with .^ after importing Gadfly #78

rdeits opened this issue Nov 14, 2016 · 19 comments
Labels

Comments

@rdeits
Copy link
Contributor

rdeits commented Nov 14, 2016

This is a weird one. Here's the typical, correct behavior:

julia> using StaticArrays

julia> s = SVector(1., 2, 3)
3-element StaticArrays.SVector{3,Float64}:
 1.0
 2.0
 3.0

julia> s .^ 2
3-element StaticArrays.SVector{3,Float64}:
 1.0
 4.0
 9.0

And here's the result after starting a new Julia shell and also importing Gadfly:

julia> using StaticArrays

julia> import Gadfly

julia> s = SVector(1., 2, 3)
3-element StaticArrays.SVector{3,Float64}:
 1.0
 2.0
 3.0

julia> s .^ 2
3-element StaticArrays.SVector{3,Any}:
 1.0
 4.0
 9.0

Note that the element type has become Any.

This is in Julia 0.5 with StaticArrays 0.1.0 and Gadfly 0.5.2 I've reproduced it on OSX and Ubuntu 14.04.

Any ideas what would cause this? The issue only shows up with .^, not with .*, .-, etc.

@andyferris
Copy link
Member

Lol. This is a funny one!

Assuming Gadfly isn't performing some kind of type piracy, it is possible that my use of promote_op is fishy, as I noticed once it was spuriously returning Any from within a generated (or maybe pure) function. If this is true, loading Gadfly would have just put the compiler into a different internal state randomly resulting in these mistakes (i.e. it's not really Gadfly's fault).

Let's check directly with the compiler gods:

@vtjnash @JeffBezanson I hope I'm not being rude, but could I ask - should promote_op, _default_type, etc (which use Core.Inference.return_type on a Generator, if memory serves) work properly from a @generated function's generator? What about from the actual generated function itself - is there a similar limitation there to comprehensions and closures?

@vtjnash
Copy link

vtjnash commented Nov 14, 2016

No, it may return the wrong answer if you attempt to call it from anywhere inside the generated function generator.

@andyferris
Copy link
Member

Thanks for confirming!

Can I also confirm that it is OK from the generated function body itself?

@vtjnash
Copy link

vtjnash commented Nov 14, 2016

Yes, there are no restrictions on the code of the generated function body.

@andyferris
Copy link
Member

Awesome, great! That helps a lot.

Yes, there are no restrictions on the code of the generated function body.

(except the comprehensions and closures...)

@vtjnash
Copy link

vtjnash commented Nov 14, 2016

Of course. That is not the code for the body, it is the code for another function.

@andyferris
Copy link
Member

OK. I think I see how that makes sense.

@andyferris
Copy link
Member

andyferris commented Nov 14, 2016

By the way, @rdeits the comprehension syntax I've implemented is only meant for literals like @SVector [i for i = 1:10]. In no case will your example compile to fast code inside a function.

We definitely want to support this behaviour (the output dimension is inferrable from the static vector inputs) but it's just not implemeted yet.

EDIT: Sorry got this issue muddled with another :)

@andyferris
Copy link
Member

@rdeits could you confirm if this is fixed on master for you, please?

@rdeits
Copy link
Contributor Author

rdeits commented Nov 15, 2016

I'm afraid it's still happening. I've pulled a430568 but I'm getting the same result.

@andyferris
Copy link
Member

Ok thanks. I'll keep looking.

@vtjnash
Copy link

vtjnash commented Nov 15, 2016

@andyferris looking at your last commit, you changed the wrong functions. It's fine to generate AST that calls promote_op; but it is very much not OK to actually attempt to call promote_op from the generator.

As food for thought, you might be able to structure your code such that the types are computed outside of the generator:

foo(...) = foo_with_eltype(promote_op(...), ...)
@generate function foo_with_eltype{T}(::Type{T}, args...)
    return quote
        ...
    end
end

@andyferris
Copy link
Member

Yes... actually the commit amounted to nothing at all but an attempt to be a bit clearer that promote_type is only called from the generated AST.

Now I'm wondering where this is going wrong?

@vtjnash
Copy link

vtjnash commented Nov 16, 2016

Ah, ok. Yes that clarification helps, since I misread its intent in passing, going off the original question in this issue.

@andyferris
Copy link
Member

Ah, ok. Yes that clarification helps, since I misread its intent in passing, going off the original question in this issue.

I was rather unclear, especially the commit message.


Hmm... so I still can reproduce this locally. I get a similar problem with broadcast(^, s, 2), which is called by .^. This function creates a closure, but is a standard (@inline) function:

@inline broadcast(f, a::StaticArray, n::Number) = map(x -> f(x, n), a)

The closure gets passed to map and there it goes into promote_op but only in the generated AST:

@generated function map{T}(f, a1::StaticArray{T})
    exprs = [:(f(a1[$j])) for j = 1:length(a1)]
    return quote
        $(Expr(:meta, :inline))
        newtype = similar_type(typeof(a1), promote_op(f, T))
        @inbounds return $(Expr(:call, :newtype, Expr(:tuple, exprs...)))
    end
end

Here it is apparent the output of similar_type is SVector{3,Any} which (in theory) should only occur if promote_op(f,T) = Any (similar_type is also a generated function but it doesn't rely on inference, but does do some type introspection and type tree traversal).

@vtjnash should promote_op work with a closure?

@andyferris
Copy link
Member

OK I've been thinking about this some more. Lately, I do seem to note fishy behaviour in Julia where sometimes inference isn't working very nicely with closures; I think there are some open issues for this.

It seems quite possible that the problem here is with map(x -> f(x,n), a), where the closure isn't being as tightly inferred as it could be, and so map's call to promote_op returns something unexpected. I'm going to treat this as a bug outside of StaticArrays until we get more information, and we can try to revisit this later.

@andyferris
Copy link
Member

I wonder if this issue presents anymore?

@KristofferC
Copy link
Contributor

I think this was fixed in 0.5.1

@c42f
Copy link
Member

c42f commented Jul 31, 2019

Let's close this then. I think we've long been aware of the perils of calling generic functions inside the generators and we try not to do it anymore (hopefully we've succeeded!)

@c42f c42f closed this as completed Jul 31, 2019
@c42f c42f added the bug label Jul 31, 2019
@c42f c42f mentioned this issue Oct 22, 2019
oschulz pushed a commit to oschulz/StaticArrays.jl that referenced this issue Apr 4, 2023
Add type parameter for index style in StructArray
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants