move promote_type call inside the generated function body #208
Conversation
| xargs = [:(xs[$d]) for d in 1:length(xs)] | ||
| :(hessian!(Array{TH, 2}($n,$n), itp, $(xargs...))) | ||
| quote | ||
| TH = $(Expr(:call, :promote_type, T, [x <: AbstractArray ? eltype(x) : x for x in xs]...)) |
There was a problem hiding this comment.
The problem is that this constructs an array and does the type computations at runtime. It may not be type-stable, either. It seems like you'd be better off doing something like TH = hessian_type(T, xs...) with
hessian_type(T) = T
hessian_type(T, x::AbstractArray, xs...) = promote_type(eltype(x), hessian_type(T, xs...))
hessian_type(T, x, xs...) = promote_type(typeof(x), hessian_type(T, xs...))that can get evaluated at compile time.
There was a problem hiding this comment.
Are you sure? That array is constructed inside the generator, not in the run-time code, and all the branching likewise happens inside the generator. I'm a fan of the lisp-y recursion trick anyway, but I'm pretty sure this case will work as-is.
There was a problem hiding this comment.
Oh, right, I misread your code — you're right, there should continue to be no runtime cost here.
|
Rebased. Good to go? |
|
Re-rebased after #197 🙂 |
Fixes #207 (at least for the particular case i showed there).
I also changed the equivalent code in
hessian(), although i think that code may have been broken already (Array{THshould have beenArray{$TH). I'll see what the test coverage says.Anyway, please let me know if you think this is the right thing to do. If so, I should be able to construct a test to make sure #207 is really fixed.