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

Type inconsistencies #984

Closed
cscherrer opened this issue Oct 1, 2019 · 5 comments
Closed

Type inconsistencies #984

cscherrer opened this issue Oct 1, 2019 · 5 comments

Comments

@cscherrer
Copy link

There are some serious type inconsistencies here:

julia> b = Bernoulli()
Bernoulli{Float64}(p=0.5)

julia> rand(b)
false

julia> rand(b,1)
1-element Array{Int64,1}:
 0

julia> eltype(b)
Int64

Can we please have rand(dist) :: T imply

  • rand(dist, n::Int) :: Array{T}
  • eltype(dist) == T
    ?
@matbesancon
Copy link
Member

Both fixes are acceptable (moving everything to Bool or to Int)

@cscherrer
Copy link
Author

cscherrer commented Oct 19, 2019

Thanks @matbesancon.

Bool seems a better fit to me, any objections (from anyone)?

Any thoughts on rand(dist, n::Int) :: Array{T}? This would require significant breaking changes for multivariate and matrix-variate distributions, but to me it seems worth it to avoid having to consider special cases.

I can understand the reasoning for the current approach: a regular array-of-vectors is more efficiently stored in a matrix. But the lack of regularity in the interface is a bit painful. It seems like we need a way to store something as a matrix but access it as an array of vectors. I thought I had seen a library that provides this capability, but I'm having trouble finding it right now. Any ideas?

@matbesancon
Copy link
Member

Bool seems a better fit to me, any objections (from anyone)?

I'm good with Bool on this, the fact that Bool is an Integer is helpful here.

Any thoughts on rand(dist, n::Int) :: Array{T}? This would require significant breaking changes for multivariate and matrix-variate distributions, but to me it seems worth it to avoid having to consider special cases.

We have to keep in mind that the standard method is always rand(RNG, ...), which fallbacks to rand(Random.GLOBAL_RNG, ...). The new rand interface would be:

rand(RNG, dist::Distribution, n::Integer) :: AbstractArray{T}

The AbstractArray subtype used is specialized for the distribution.

It seems like we need a way to store something as a matrix but access it as an array of vectors. I thought I had seen a library that provides this capability, but I'm having trouble finding it right now. Any ideas?

RecursiveArrayTools.jl could do the job

@cscherrer
Copy link
Author

RecursiveArrayTools.jl could do the job

I had thought this too, but it seems to solve the problem of treating a vector-of-vectors as a matrix, while we need to go the other way.

A few years ago I saw a nice approach in Haskell that gave a nice way to build abstractions on customized efficient internal representations. I'll see if I can find it, in case it's not too Haskell-specific; maybe some of the ideas can apply here

@matbesancon
Copy link
Member

This would require significant breaking changes for multivariate and matrix-variate distributions

If it improves consistency it's fine, we can always add breaking changes

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