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

Remove Distributions #3071

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Remove Distributions #3071

merged 1 commit into from
Jun 6, 2024

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Jun 6, 2024

    xp_low = Distributions.quantile(Distributions.Normal(), low_percentile)
    xp_high = Distributions.quantile(Distributions.Normal(), high_percentile)
    return (gauss_int(xp_high) - gauss_int(xp_low)) / max(
        Distributions.cdf(Distributions.Normal(), xp_high) -
        Distributions.cdf(Distributions.Normal(), xp_low),
        eps(FT),
    )

Part of this block is doing nothing.

Distributions.quantile(percentile) computes the x so that the CDF is percentile. Then, Distributions.cdf(x) computes the percentile corresponding to x. The two functions are one the inverse of the other when the same distribution is used.

So, all we need to remove distributions is implement the inverse error function. I used one of the appromation listed on Wikipedia:
image

Closes #3046

@Sbozzolo Sbozzolo requested a review from szy21 June 6, 2024 16:41
@Sbozzolo Sbozzolo force-pushed the gb/remove_distributions branch 4 times, most recently from 7f17cd4 to e3939d0 Compare June 6, 2024 16:59
Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

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

Thanks! The code change looks good to me. I didn't check the math but I trust you and wiki:) Could you check what is the result for percentile_bounds_mean_norm(0.9, 1), and maybe add a test? (I can add the test later if you are busy with other things.)

@Sbozzolo Sbozzolo force-pushed the gb/remove_distributions branch from e3939d0 to 57813e3 Compare June 6, 2024 19:31
@Sbozzolo
Copy link
Member Author

Sbozzolo commented Jun 6, 2024

Thanks! The code change looks good to me. I didn't check the math but I trust you and wiki:) Could you check what is the result for percentile_bounds_mean_norm(0.9, 1), and maybe add a test? (I can add the test later if you are busy with other things.)

I added a test, I just forgot to commit it!

@Sbozzolo Sbozzolo enabled auto-merge June 6, 2024 19:32
Distributions is a heavy package, but all we need is the inverse error
function. Wikipedia teaches us that there are relatively simple
approximations for that:

https://www.academia.edu/9730974/A_handy_approximation_for_the_error_function_and_its_inverse
@Sbozzolo Sbozzolo force-pushed the gb/remove_distributions branch from 57813e3 to 68dba3c Compare June 6, 2024 19:53
@Sbozzolo Sbozzolo added this pull request to the merge queue Jun 6, 2024
Merged via the queue into main with commit bfc8b56 Jun 6, 2024
9 of 11 checks passed
@Sbozzolo Sbozzolo deleted the gb/remove_distributions branch June 6, 2024 21:49
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.

Remove Distributions.jl
2 participants