Von mises range fix #1280
Von mises range fix #1280
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1280 +/- ##
==========================================
- Coverage 81.34% 79.38% -1.97%
==========================================
Files 117 117
Lines 6563 5977 -586
==========================================
- Hits 5339 4745 -594
- Misses 1224 1232 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I added some comments regarding the implementation.
However, more generally I am not sure if the pdf and logpdf should be changed in this way. The convention is that the pdf is zero outside of the support of the distribution, so the definition of support and pdf/logpdf should be consistent. Also it seems inconsistent if the pdf does not imtegrate up to 1 anymore. Additionally, it would be inconsistent with cdf etc.
| @@ -193,6 +193,7 @@ export | |||
| isplatykurtic, # Is excess kurtosis > 0.0? | |||
| isleptokurtic, # Is excess kurtosis < 0.0? | |||
| ismesokurtic, # Is excess kurtosis = 0.0? | |||
| isperiodic, # Has a periodic pdf? | |||
There was a problem hiding this comment.
This seems like a very special case, so currently I think it should not be part of the API. And hence also not exported.
| @@ -58,6 +58,7 @@ get_subtypes(x::Type) = _subtypes_in(Base.loaded_modules_array(), x) | |||
| dists = get_subtypes(UnivariateDistribution) | |||
| filter!(x -> hasmethod(x, ()), dists) | |||
| filter!(x -> isbounded(x()), dists) | |||
| filter!(x -> !isperiodic(x()), dists) | |||
There was a problem hiding this comment.
Just filter VonMises here if necessary?
| pdf(d::VonMises{T}, x::Real) where T<:Real = exp(d.κ * (cos(x - d.μ) - 1)) / (twoπ * d.I0κx) | ||
| logpdf(d::VonMises{T}, x::Real) where T<:Real = d.κ * (cos(x - d.μ) - 1) - log(d.I0κx) - log2π |
There was a problem hiding this comment.
| pdf(d::VonMises{T}, x::Real) where T<:Real = exp(d.κ * (cos(x - d.μ) - 1)) / (twoπ * d.I0κx) | |
| logpdf(d::VonMises{T}, x::Real) where T<:Real = d.κ * (cos(x - d.μ) - 1) - log(d.I0κx) - log2π | |
| pdf(d::VonMises, x::Real) = exp(d.κ * (cos(x - d.μ) - 1)) / (twoπ * d.I0κx) | |
| logpdf(d::VonMises, x::Real) = d.κ * (cos(x - d.μ) - 1) - log(d.I0κx) - log2π |
|
It is frustrating that this PR (and other PR) related to VonMises are not resolved and merged. The current implementation gives unexpected results. It seems that the simplest solution to these issues is to define support on [-pi , pi] and then wrap input (to pdf or cdf) to this range and likewise, always give samples in this range. |
|
I reviewed the PR and it seems the PR is clearly not in a mergeable state. The comments have to be addressed first but apparently this has not been done. |
This pull request reverts part of #965 which added bound checking to the pdf of several distributions.
This shouldn't be done for the von Mises distribution as it is designed to be periodic, repeating every 2π, so values for the pdf beyond the support of the distribution are entirely valid.
To enable the univariate_bounds tests to continue working with as few changes as possible, this also adds an
isperiodic()method which is set to false in univariates.jl and then set to true in vonmises.jl