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

Uniform overhaul #1045

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Uniform overhaul #1045

wants to merge 3 commits into from

Conversation

aminya
Copy link

@aminya aminya commented Dec 31, 2019

Fixes #1041

  • rand(Uniform) is now type stable
  • Bounds types are independent of distribution type

Alternative branch: UniformOverhaul2

@aminya
Copy link
Author

aminya commented Dec 31, 2019

If you think that we should keep the type of a and b the same internally (may help speed), this definition also works:

UniformOverhaul2

struct Uniform{T} <: ContinuousUnivariateDistribution
    a::T
    b::T

    # Uniform{T}(a::T, b::T) constructor
    function Uniform{T}(a::T, b::T; check_args=true) where {T <: Real}
        check_args && @check_args(Uniform, a < b)
        return new{T}(a, b)
    end

    # Abstract a,b - Uniform{T}(a, b) constructor
    function Uniform{T}(a::Real, b::Real; check_args=true) where {T <: Real}
        check_args && @check_args(Uniform, a < b)
        return new{T}(T(a), T(b))
    end

    # Uniform{T}(a::T, b::T) constructor
    function Uniform{T}(a::T, b::T) where {T <:Complex}
        new{T}(a, b)
    end

    # Abstract a,b - Uniform{T}(a, b) constructor
    function Uniform{T}(a, b) where {T <: Complex}
        return new{T}(T(a), T(b))
    end
end
# Real

# zeros() like constructor (Uniform(T, a::T, b::T))
function Uniform(::Type{T}, a::T, b::T; check_args=true) where {T <: Real}
    return Uniform{T}(a, b, check_args = check_args)
end

# Abstract a,b - zeros() like constructor (Uniform(T, a, b))
function Uniform(::Type{T}, a, b; check_args=true) where {T <: Real}
    return Uniform{T}(T(a), T(b), check_args = check_args)
end

# No type specified constructor:
function Uniform(a::Float64, b::Float64; check_args=true)
    return Uniform{Float64}(a, b, check_args = check_args)
end

# Abstract a,b - no type specified constructor:
function Uniform(a, b; check_args=true)
    return Uniform{Float64}(Float64(a), Float64(b), check_args = check_args)
end

# Complex

# zeros() like constructor (Uniform(T, a::T, b::T))
function Uniform(::Type{T}, a::T, b::T) where {T <: Complex}
    return Uniform{T}(a, b)
end

# Abstract a,b - zeros() like constructor (Uniform(T, a, b))
function Uniform(::Type{T}, a, b) where {T <: Complex}
    return Uniform{T}(T(a), T(b))
end

# No type specified constructor:
function Uniform(a::ComplexF64, b::ComplexF64)
    return Uniform{ComplexF64}(a, b)
end

@codecov-io
Copy link

codecov-io commented Dec 31, 2019

Codecov Report

Merging #1045 into master will increase coverage by <.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1045      +/-   ##
==========================================
+ Coverage   78.31%   78.31%   +<.01%     
==========================================
  Files         112      112              
  Lines        5349     5350       +1     
==========================================
+ Hits         4189     4190       +1     
  Misses       1160     1160
Impacted Files Coverage Δ
src/univariate/continuous/uniform.jl 86% <85.71%> (+0.28%) ⬆️

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 033463e...3d9f749. Read the comment docs.

@andreasnoack
Copy link
Member

I don't support the changes here. Adding the a Uniform{T}(a::Real, b::Real) = Uniform(T(a), T(b)) constructor seems fine but I don't see the need for the remaining changes.

#1041 raises a discussion about the type returned by rand. I don't think we have made a decision about what the behavior should be but it seems like we currently return Float64 for most distributions regardless of the parameter type. An exception is the Normal distribution where we rand uses the type of the parameters. I think the difference is unintended.

We should probably open a separate issue for discussing this or simply change the title of #1041 to cover all distributions. I see four possible behaviors for rand

  1. a. Unconditionally construct Float64s
  2. b. Add a type argument to rand to specify the variate type but default to Float64
  3. Return variates of the same type as the parameters
  4. Introduce a separate type parameter for the variate type

I think 3. will add too much overhead without a real demand. I think I'd prefer one of the 1. options.

@cossio
Copy link
Contributor

cossio commented Mar 7, 2020

I think 1a (always returning Float64) is too restrictive. Sometimes we want to work with Float32 for performance or memory limitations.

2b (adding a type parameter to rand) could work.

But it seems that 3., returning types following the parameters, is the most natural solution here. Especially since many distributions have "location" or "scale" parameters, which in a sense carry the "units" of the samples. I think this is what users expect (a couple of issues are requesting just this). I don't understand why this would be much more difficult than the 2b. approach?

@rafaqz
Copy link

rafaqz commented Aug 25, 2020

My five cents is also that option 3 is the most natural solution. I just attempted to switch a complex simulation over to Float32 for performance and Distributions.jl was the only blocker to that just working with no code changes.

Having to add an argument specifically to allow Float32 seems unnecessary both for users and devs, when the parameters already contain the type information.

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.

Uniform is not type stable + Bounds type should be independent of distribution type
5 participants