Fix undefined reference error when type widening

[mohamed82008]

While testing stochastic control flow in Turing and DynamicPPL, I ran into an issue where BangBang.setindex!! tries to copy all of the old vector of transitions. Some of the elements of this vector would be undefined so it would throw and undefined reference error. This PR uses BangBang.push!! and sizehint! instead.

[codecov]

Codecov Report

Merging #40 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   97.54%   97.61%   +0.07%     
==========================================
  Files           5        5              
  Lines         122      126       +4     
==========================================
+ Hits          119      123       +4     
  Misses          3        3              

Impacted Files	Coverage Δ
src/interface.jl	93.33% <100.00%> (+2.42%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7efc8a...ccb13cb. Read the comment docs.

[devmotion]

Do you have a MWE? IMO we should keep the interface and implementation in AbstractMCMC as general as possible, and as pointed out above the changes are quite restrictive. Maybe the underlying issue of the undefined reference could be fixed differently? Downstream packages can also always add their own implementation of transitions (hopefully only for samplers or models that they own...).

[mohamed82008]

Here is a MWE:

julia> struct M <: AbstractMCMC.AbstractModel end

julia> struct S <: AbstractMCMC.AbstractSampler end

julia> a = Vector{Base.RefValue{Int}}(undef, 5); AbstractMCMC.save!!(a, Ref(1.0), 1, M(), S(), 5)
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getindex at .\array.jl:787 [inlined]
 [2] copyto!(::Array{Base.RefValue,1}, ::Int64, ::Array{Base.RefValue{Int64},1}, ::Int64, ::Int64) at .\abstractarray.jl:841
 [3] append! at .\array.jl:955 [inlined]
 [4] copy! at .\abstractarray.jl:708 [inlined]
 [5] _setindex(::Array{Base.RefValue{Int64},1}, ::Base.RefValue{Float64}, ::Int64) at C:\Users\user\.julia\dev\BangBang\src\NoBang\base.jl:135
 [6] may at C:\Users\user\.julia\dev\BangBang\src\core.jl:10 [inlined]
 [7] setindex!! at C:\Users\user\.julia\dev\BangBang\src\base.jl:365 [inlined]
 [8] #save!!#11 at C:\Users\user\.julia\dev\AbstractMCMC\src\interface.jl:139 [inlined]
 [9] save!!(::Array{Base.RefValue{Int64},1}, ::Base.RefValue{Float64}, ::Int64, ::M, ::S, ::Int64) at C:\Users\user\.julia\dev\AbstractMCMC\src\interface.jl:139
 [10] top-level scope at REPL[23]:1
 [11] eval(::Module, ::Any) at .\boot.jl:331
 [12] eval_user_input(::Any, ::REPL.REPLBackend) at C:\Users\user\AppData\Local\Programs\Julia\Julia-1.4.0\share\julia\stdlib\v1.4\REPL\src\REPL.jl:86
 [13] run_backend(::REPL.REPLBackend) at C:\Users\user\.julia\packages\Revise\MgvIv\src\Revise.jl:1023
 [14] top-level scope at REPL[1]:0

[devmotion]

Do you need to work with Ref? Or could you initialize the first transition in the correct way?

[mohamed82008]

Do you need to work with Ref? Or could you initialize the first transition in the correct way?

I don't work with Ref but with Transition. Ref is just a stand-in. Even if you initialize the first element properly, you get the same error when it tries to widen the type of the array because copy!(b, a) is called where a is the partially undefined vector which throws the above error. So we can either drop the undef definition or we use push!! instead of setindex!!.

[devmotion]

So we can either drop the undef definition or we use push!! instead of setindex!!.

Yeah, I guess we could just switch to push!! in general and get rid of the preallocation. That would also be closer to a transducer-based approach at some point (see #39).

Can you revert the changes and just use push!! in

AbstractMCMC.jl/src/interface.jl

Line 139 in c7efc8a

return BangBang.setindex!!(transitions, transition, iteration)

(and adjust the docstring), change

AbstractMCMC.jl/src/interface.jl

Line 106 in c7efc8a

return Vector{typeof(transition)}(undef, N)

to

    t = Vector{typeof(transition)}(undef, 0)
    sizehint!(t, N)
    return t

and (while we're at it)

AbstractMCMC.jl/src/interface.jl

Line 115 in c7efc8a

return Vector{typeof(transition)}(undef, 1)

to

    return Vector{typeof(transition)}(undef, 0)

? Then we keep the general interface but can optimize it still a bit for the default case of vectors.

[mohamed82008]

Sounds good to me.

[cpfiffer]

@devmotion I'm fine with this one if you are.

[devmotion]

I updated the docstring and restricted the default implementation to containers that are Vectors. Looks good to me if tests pass.