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

allow specify parameter type in AffineDistribution #1769

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

Conversation

bgctw
Copy link
Contributor

@bgctw bgctw commented Sep 7, 2023

second suggestion to workaround #1765

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
src/univariate/continuous/logitnormal.jl ø
src/univariate/locationscale.jl 100.00%

📢 Thoughts on this report? Let us know!.

Comment on lines +75 to +77
# # 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)
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)

@@ -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

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)

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.

3 participants