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

Undocumented or unintended change in interface for defining distributions #846

Closed
s-broda opened this issue Mar 22, 2019 · 4 comments
Closed

Comments

@s-broda
Copy link

s-broda commented Mar 22, 2019

The following code works on 0.16.4

using Distributions
using Random
import Random: rand
using StatsFuns.RFunctions: tdistrand
export StdT
"""
    StdT <: Distribution{Univariate, Continuous}
Standardized t distribution.
"""
struct StdT <: Distribution{Univariate, Continuous}
    v::Float64
end
rand(d::StdT)  =  tdistrand(d.v)*sqrt((d.v-2)/d.v)
Random.seed!(1)
@show rand(StdT(4), 1)
end

but errors on 0.17.0 with

ERROR: LoadError: MethodError: no method matching iterate(::Main.Repro.StdT)
Closest candidates are:
  iterate(::Core.SimpleVector) at essentials.jl:568
  iterate(::Core.SimpleVector, ::Any) at essentials.jl:568
  iterate(::ExponentialBackOff) at error.jl:199
  ...
Stacktrace:
 [1] copyto!(::Array{Float64,1}, ::Main.Repro.StdT) at .\abstractarray.jl:668
 [2] _collect(::UnitRange{Int64}, ::Main.Repro.StdT, ::Base.HasEltype, ::Base.HasLength) at .\array.jl:550
 [3] collect(::Main.Repro.StdT) at .\array.jl:544
 [4] #quantile#51(::Bool, ::Function, ::Main.Repro.StdT, ::Float64) at C:\cygwin\home\Administrator\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.1\Statistics\src\Statistics.jl:916
 [5] quantile(::Main.Repro.StdT, ::Float64) at C:\cygwin\home\Administrator\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.1\Statistics\src\Statistics.jl:916
 [6] rand(::Random.MersenneTwister, ::Main.Repro.StdT) at C:\Users\broda\.julia\packages\Distributions\fMt8c\src\univariates.jl:177
 [7] rand! at C:\Users\broda\.julia\packages\Distributions\fMt8c\src\univariates.jl:167 [inlined]
 [8] rand at C:\Users\broda\.julia\packages\Distributions\fMt8c\src\univariates.jl:160 [inlined]
 [9] rand(::Main.Repro.StdT, ::Int64) at C:\Users\broda\.julia\packages\Distributions\fMt8c\src\genericrand.jl:24
 [10] top-level scope at show.jl:555
in expression starting at C:\Users\broda\Documents\julia\local_packages\Repro\src\Repro.jl:17

Conversely, changing

rand(d::StdT)  =  tdistrand(d.v)*sqrt((d.v-2)/d.v)

to

rand(RNG::Random.AbstractRNG, d::StdT)  =  tdistrand(d.v)*sqrt((d.v-2)/d.v)

works on v0.17.0, but fails on v0.16.4, with the same error. The code works on both versions if both definitions are present.

I can't seem to find anything in the docs about this change.
Thanks!
Simon

@matbesancon
Copy link
Member

ok yes this is an issue indeed, sorry about that breaking change. We forgot to release in a long time, which is far from ideal

@matbesancon
Copy link
Member

@s-broda digging into what happened, the problem here is that

rand(::StdT, ::Int)

Will boil down to:

rand(::RNG, ::StdT, n)

which will not re-dispatch to rand(d::StdT)

So the problem is that we have a change of required interface to implement: rand(::AbstractRNG, ::YourDist)

@matbesancon
Copy link
Member

Mostly a bug from 0.16 I would say, and the error should be more meaningful. If you want to open a PR, I'd be 100% for it

@matbesancon
Copy link
Member

So there could be an error on calling quantile(::Dist, p) by default to force an implementation

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

No branches or pull requests

2 participants