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

Native pdf, cdf, quantile, and related functions for the Normal distribution #714

Closed
wants to merge 5 commits into from

Conversation

Nosferican
Copy link
Contributor

Porting the native implementation from StatsFuns to Distributions. See #708

zval(μ::Real, σ::Real, x::Number) = (x - μ) / σ

# pdf
normpdf(z::Real) = exp(-abs2(z)/2) * invsqrt2π
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should implement the normX versions.

Copy link
Member

Choose a reason for hiding this comment

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

So rework without going through the normalization step?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should just define these for the Distributions API, i.e. this method would simply be pdf(::Normal, z)

Copy link
Member

@simonbyrne simonbyrne Mar 1, 2019

Choose a reason for hiding this comment

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

It could be useful to have a separate "basic" functions. i.e. normpdf(z), but we don't need normpdf(x,mu,sigma) versions.

similarly, normcdf, but you don't need normccdf since this is trivially defined in terms of normcdf.

#### Evaluation
#### Evaluation (see JuliaStats/Distributions.jl/issues/708)

xval(μ::Real, σ::Real, z::Number) = μ + σ * z
Copy link
Member

Choose a reason for hiding this comment

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

are xval and zval helper functions? Should these get inlined?

zval(μ::Real, σ::Real, x::Number) = (x - μ) / σ

# pdf
normpdf(z::Real) = exp(-abs2(z)/2) * invsqrt2π
Copy link
Member

Choose a reason for hiding this comment

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

So rework without going through the normalization step?

# Journal of the Royal Statistical Society. Series C (Applied Statistics), Vol. 37, No. 3, pp. 477-484
#

function _norminvlogcdf_impl(lp::Float64)
Copy link
Member

Choose a reason for hiding this comment

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

even if not exported, I would be in favour of adding add docstrings to non-trivial functions


@_delegate_statsfuns Normal norm μ σ
function _qnorm_ker1(q::Float64)
Copy link
Member

Choose a reason for hiding this comment

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

same thing here, a docstring would help

5.22649_52788_52854_5610e3)
end

function _qnorm_ker2(r::Float64)
Copy link
Member

Choose a reason for hiding this comment

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

same there

@matbesancon
Copy link
Member

@Nosferican updates on this?

@Nosferican
Copy link
Contributor Author

I had forgotten about this. I will try to finish it up next I find a break. Thanks for reminding me.

@matbesancon
Copy link
Member

ping @Nosferican

@Nosferican
Copy link
Contributor Author

I know I have been lagging, but soon I will have time to go back to this. I am defending in two weeks so probably check with me a bit after that. If anyone else wants to take a go in the meantime, feel free.

@matbesancon
Copy link
Member

Hi @Nosferican, congrats on the defense :)
Up-ing on this

@matbesancon
Copy link
Member

also, there might be some merge to do first, and addressing the comments

@Nosferican
Copy link
Contributor Author

Thanks! Seems like the branch I used for the PR was deleted sometime within the last year... I will close this PR and open a fresh one with the comments addressed.

@Nosferican Nosferican closed this Apr 25, 2019
@Nosferican Nosferican mentioned this pull request Apr 25, 2019
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.

4 participants