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

[Merged by Bors] - Fix for type-instability in findranges #277

Closed
wants to merge 2 commits into from

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented Jul 14, 2021

TuringLang/Turing.jl#1661

With this PR:

julia> using Turing, DynamicPPL, ReverseDiff

julia> Turing.setadbackend(:reversediff);

julia> @model function demo(x, ::Type{TV} = Vector{Float64}) where {TV}
           m = TV(undef, length(x))
           m ~ MvNormal(length(x), 1.0)
           x ~ MvNormal(m, 1.0)
       end;

julia> m = demo(randn(2));

julia> spl = DynamicPPL.Sampler(NUTS(0.65))
Sampler{NUTS{Turing.Core.ReverseDiffAD{false}, (), AdvancedHMC.DiagEuclideanMetric}}(NUTS{Turing.Core.ReverseDiffAD{false}, (), AdvancedHMC.DiagEuclideanMetric}(-1, 0.65, 10, 1000.0, 0.0), DynamicPPL.Selector(0x000f976df8fc2a48, :default, false))

julia> vi = DynamicPPL.VarInfo(m);

julia> θ = vi[spl];

julia> new_vi = VarInfo(vi, spl, ReverseDiff.track(θ));

julia> eltype(new_vi, spl) # (✓) inference succeeds
ReverseDiff.TrackedReal{Float64, Float64, ReverseDiff.TrackedArray{Float64, Float64, 1, Vector{Float64}, Vector{Float64}}}

@torfjelde
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jul 14, 2021
@torfjelde torfjelde requested a review from yebai July 14, 2021 15:42
@inline function findranges(f_ranges, f_idcs)
return mapreduce(i -> f_ranges[i], vcat, f_idcs; init=Int[])
results = Int[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the size of results array is known, maybe pre-allocate memory for results here to improve efficiency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not known though, since these are ranges that being concatenated, not just 1 Int per element in f_ranges. We could of course compute it by doing something like map(length, f_ranges), but can we always guarantee that f_idcs holds all the f_ranges? I wasn't sure about this, so preferred to leave it with size not specified.

@yebai
Copy link
Member

yebai commented Jul 14, 2021

Bors r+

bors bot pushed a commit that referenced this pull request Jul 14, 2021
TuringLang/Turing.jl#1661

With this PR:

```julia
julia> using Turing, DynamicPPL, ReverseDiff

julia> Turing.setadbackend(:reversediff);

julia> @model function demo(x, ::Type{TV} = Vector{Float64}) where {TV}
           m = TV(undef, length(x))
           m ~ MvNormal(length(x), 1.0)
           x ~ MvNormal(m, 1.0)
       end;

julia> m = demo(randn(2));

julia> spl = DynamicPPL.Sampler(NUTS(0.65))
Sampler{NUTS{Turing.Core.ReverseDiffAD{false}, (), AdvancedHMC.DiagEuclideanMetric}}(NUTS{Turing.Core.ReverseDiffAD{false}, (), AdvancedHMC.DiagEuclideanMetric}(-1, 0.65, 10, 1000.0, 0.0), DynamicPPL.Selector(0x000f976df8fc2a48, :default, false))

julia> vi = DynamicPPL.VarInfo(m);

julia> θ = vi[spl];

julia> new_vi = VarInfo(vi, spl, ReverseDiff.track(θ));

julia> eltype(new_vi, spl) # (✓) inference succeeds
ReverseDiff.TrackedReal{Float64, Float64, ReverseDiff.TrackedArray{Float64, Float64, 1, Vector{Float64}, Vector{Float64}}}

```
@bors bors bot changed the title Fix for type-instability in findranges [Merged by Bors] - Fix for type-instability in findranges Jul 14, 2021
@bors bors bot closed this Jul 14, 2021
@bors bors bot deleted the tor/type-instability-fix branch July 14, 2021 19:09
@torfjelde torfjelde mentioned this pull request Jul 19, 2021
3 tasks
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

Successfully merging this pull request may close these issues.

2 participants