add partype method to lognormal and semicircle #1773
Conversation
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
There was a problem hiding this comment.
It would be good to add a check for partype to the automatic distribution test suite (at least for univariate distributions), e.g., in
Distributions.jl/test/univariates.jl
Line 51 in 2cd6c67
partype(d) === Base.promote_typeof(params...) and partype(d) === Float32 in the Float32 conversion checks.
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
of nothing parameters and fix warning on ambiguous global variable
|
I implemented the tests for all distributions suggested by @devmotion. They cause the following problems.
|
but promote eltype T with eltype(inner) but also promote partype T with partype(inner)
| @@ -73,7 +87,7 @@ modes(d::DiscreteUniform) = [d.a:d.b] | |||
| pdf(d::DiscreteUniform, x::Real) = insupport(d, x) ? d.pv : zero(d.pv) | |||
| logpdf(d::DiscreteUniform, x::Real) = log(pdf(d, x)) | |||
|
|
|||
| function cdf(d::DiscreteUniform, x::Int) | |||
| function cdf(d::DiscreteUniform, x::Integer) | |||
There was a problem hiding this comment.
Unrelated?
| function cdf(d::DiscreteUniform, x::Integer) | |
| function cdf(d::DiscreteUniform, x::Int) |
There was a problem hiding this comment.
Will adapt this when reverting the DiscreteUniform to non-parametric.
test/univariates.jl
Outdated
| # truncated parameters may be nothing: Union{Nothing, promote_type()} | ||
| # but partype should still be type of the non-nothing ones | ||
| #@test partype(d) === promote_type(typeof.(pars)...) | ||
| @test partype(d) === promote_type(typeof.(pars)...) || | ||
| Union{Nothing,partype(d)} === promote_type(typeof.(pars)...) |
There was a problem hiding this comment.
Maybe just use something like
| # truncated parameters may be nothing: Union{Nothing, promote_type()} | |
| # but partype should still be type of the non-nothing ones | |
| #@test partype(d) === promote_type(typeof.(pars)...) | |
| @test partype(d) === promote_type(typeof.(pars)...) || | |
| Union{Nothing,partype(d)} === promote_type(typeof.(pars)...) | |
| # truncated parameters may be nothing: Union{Nothing, promote_type()} | |
| # but partype should still be type of the non-nothing ones | |
| @test partype(d) === mapfoldl(typeof, (S, T) -> T <: Real ? promote_type(S, T) : S, pars; init = Bool) |
There was a problem hiding this comment.
Had to wrap my head around, but implemented and tested this more general implementation with an own commit.
Co-authored-by: David Widmann <[email protected]>
will be moved to its own pull-request
e.g. Nothing in Truncated distribution
D is not used inside the function any more -> can simpify Co-authored-by: David Widmann <[email protected]>
|
Based on the code comments of @devmotion, I reverted the changes to discreteuniform. Originally, I introduced those modifications to discreteuniform to test how in addition to Float64/Float32 Distribution can also use a type parameter to allow using Int64/Int32. Since, this blows up the pull-request, I will create an PR after this PR is hopefully accepted. |
|
Do I still need to do something to move this PR forward? |
Notice that LogNormal was missing the partype method. Hence, I added it and checked all uniform/continuous distributions. If they were parametric, I added the partype method for them.