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.

Sign up for GitHub

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

Jump to bottom

allow specify parameter type in AffineDistribution #1769

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

allow specify parameter type in AffineDistribution #1769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
202edde
allow specify parameter type in AffineDistribution
bgctw Sep 7, 2023
d3a9906
test user-specified type
bgctw Sep 7, 2023
c7a64c8
make mim of LogitNormal respect parametric type
bgctw Sep 7, 2023
2ee5bee
exclude testing type of minimum
bgctw Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/univariate/continuous/logitnormal.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ LogitNormal(μ::Real=0.0) = LogitNormal(μ, one(μ); check_args=false)
@distr_support LogitNormal 0.0 1.0
#insupport(d::Union{D,Type{D}},x::Real) where {D<:LogitNormal} = 0.0 < x < 1.0

# # TODO change @distr_support to generate functions that respect the type parameter
# minimum(::LogitNormal{T}) where T = T(0)
# maximum(::LogitNormal{T}) where T = T(1)
Comment on lines +75 to +77
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# # TODO change @distr_support to generate functions that respect the type parameter
# minimum(::LogitNormal{T}) where T = T(0)
# maximum(::LogitNormal{T}) where T = T(1)


#### Conversions
convert(::Type{LogitNormal{T}}, μ::S, σ::S) where
Expand Down
15 changes: 12 additions & 3 deletions src/univariate/locationscale.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,29 @@ struct AffineDistribution{T<:Real, S<:ValueSupport, D<:UnivariateDistribution{S}
end
end

function AffineDistribution(μ::T, σ::T, ρ::UnivariateDistribution; check_args::Bool=true) where {T<:Real}
function AffineDistribution(μ::T, σ::T, ρ::UnivariateDistribution, _T::Type; check_args::Bool=true) where {T<:Real}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this approach - if users want to set the types specifically, they can just call one of the internal constructors.

Can't we just replace line 54 with

_T = promote_type(partype(ρ), T)

BTW note that Type is one of the special cases where the Julia compiler does not specialize on the argument. If you want to specialize on the type, then you have to use ::Type{_T}.

Copy link
Contributor Author

@bgctw bgctw Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @devmotion for this aid.

I already tried making AffineTransformation using partype instead of eltype and it caused problems/failing tests with the discretenonparametric distribution.

I can prepare a pull request with the change and we can look, whether the failing discretenonparametric tests are reasonable (expecting rationals that are promoted to float with the change)

The deeper problem to me is that AffineTransformation transforms both, parameter-based quantities and samples. I think we should keep these two types separate (#1433)

#workaround ##1765 to allow specifying Float32 as parametric type
@check_args AffineDistribution (σ, !iszero(σ))
_T = promote_type(eltype(ρ), T)
return AffineDistribution{_T}(_T(μ), _T(σ), ρ)
end

function AffineDistribution(μ::T, σ::T, ρ::UnivariateDistribution; check_args::Bool=true) where {T<:Real}
#@check_args AffineDistribution (σ, !iszero(σ))
_T = promote_type(eltype(ρ), T)
#_T = promote_type(partype(ρ), T) breakes DiscreteNonParametric
return AffineDistribution(μ, σ, ρ, _T)
end

function AffineDistribution(μ::Real, σ::Real, ρ::UnivariateDistribution; check_args::Bool=true)
return AffineDistribution(promote(μ, σ)..., ρ; check_args=check_args)
end

# aliases
const LocationScale{T,S,D} = AffineDistribution{T,S,D}
function LocationScale(μ::Real, σ::Real, ρ::UnivariateDistribution; check_args::Bool=true)
Base.depwarn("`LocationScale` is deprecated. Use `+` and `*` instead", :LocationScale)
Base.depwarn(
"`LocationScale` is deprecated. Use `+` and `*` instead " *
"(see ?Distributions.AffineDistribution)", :LocationScale)
# preparation for future PR where I remove σ > 0 check
@check_args LocationScale (σ, σ > zero(σ))
return AffineDistribution(μ, σ, ρ; check_args=false)
Expand Down
10 changes: 10 additions & 0 deletions test/univariate/locationscale.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,13 @@ end
@test ls_norm isa LocationScale{Float64, Continuous, Normal{Float64}}
@test ls_norm isa Distributions.AffineDistribution{Float64, Continuous, Normal{Float64}}
end


@testset "user specified type Float32" begin
dln = LogitNormal(0f0, sqrt(2f0))
lower, upper = 1f0, 3f0
d = Distributions.AffineDistribution(lower, (upper -lower), dln, partype(dln))
@test scale(d) isa Float32
#@test minimum(d) isa Float32 # minimum is more complicated
#plot(d)
end;