Use Julia implementations of `normpdf` etc. in StatsFuns #1487

devmotion · 2022-01-22T00:48:48Z

This PR replaces the implementations of pdf(::Normal, ::Real) etc. with calls to StatsFuns.normpdf etc. which are basically the same Julia implementations.

This removes code duplication (which e.g. means that one would not have to define ChainRules derivatives in both packages but it is sufficient to do so in StatsFuns) and it actually fixes subtle bugs such as reported in #1067 (comment) and arising from undesired promotions in Normal(mu, sigma) calls in the current implementation in Distributions.

Tests require JuliaStats/StatsFuns.jl#132 which fixes a bug in the StatsFuns implementation covered by our tests.

Project.toml

sethaxen

This seems a good idea in general. Although it looks like StatsBase's normal quantile function promotes to Float64 unnecessarily (see failures).

src/univariate/continuous/normal.jl

Project.toml

devmotion · 2022-01-24T22:47:27Z

Should we bump the Julia bound to 1.3 or 1.6? The test errors on Julia 1.0 are caused by an ancient version of SpecialFunctios (0.8 😮) that does not contain implementations of erfcinv(::Rational) (introduced in SpecialFunctions 1.2.0). All more recent versions of SpecialFunctions require Julia 1.3, hence for this specific issue it would be sufficient to drop Julia < 1.2.

devmotion · 2022-01-24T23:34:11Z

Alternatively, we could just not run these failing tests on Julia < 1.3.

codecov-commenter · 2022-01-25T14:07:51Z

Codecov Report

Merging #1487 (86113fd) into master (6f74846) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1487      +/-   ##
==========================================
+ Coverage   84.80%   84.92%   +0.12%     
==========================================
  Files         126      128       +2     
  Lines        7778     7735      -43     
==========================================
- Hits         6596     6569      -27     
+ Misses       1182     1166      -16

Impacted Files	Coverage Δ
src/univariate/continuous/normal.jl	`98.23% <100.00%> (-0.73%)`	⬇️
src/samplers.jl	`100.00% <0.00%> (ø)`
src/Distributions.jl	`100.00% <0.00%> (ø)`
src/quantilealgs.jl	`82.60% <0.00%> (+0.19%)`	⬆️
src/common.jl	`79.79% <0.00%> (+0.20%)`	⬆️
src/matrixvariates.jl	`95.65% <0.00%> (+0.41%)`	⬆️
src/univariates.jl	`74.07% <0.00%> (+2.64%)`	⬆️
src/multivariates.jl	`42.85% <0.00%> (+4.39%)`	⬆️
src/utils.jl	`92.42% <0.00%> (+28.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f74846...86113fd. Read the comment docs.

devmotion · 2022-02-01T00:07:05Z

OK, this PR is ready for review @mschauer @sethaxen. I decided to change the Julia bound to 1.3 and SpecialFunctions to 1.2. These changes ensure that there are no regressions for environments that use older Julia or older SpecialFunctions versions.

sethaxen

LGTM!

devmotion · 2022-02-02T16:04:10Z

Thanks for merging the PR @mschauer!

I guess Distributions doesn't follow the ColPrac guidelines officially but maybe we should adopt the policy that PR authors with merge rights merge their own PRs. Even after a PR is approved I sometimes notice an issue that has to be addressed or something minor that I'd like to change (wasn't the case here 🙂), and then it's nice if the PR is not merged yet and I can update it.

alyst · 2022-02-22T21:58:03Z

Is it intentional that this PR removed zval(), xval() and potentially something else?

devmotion · 2022-02-22T22:00:32Z

Yes. xval and zval are not used anymore.

alyst · 2022-02-22T22:01:32Z

But are they still exported and defined for many other distributions?
Edit: Ok, not exported, but still defined.

devmotion · 2022-02-22T22:06:48Z

They are just internal helpers that simplify the code a bit and make it more readable. BTW StatsFuns uses internal helpers of the same name: https://github.com/JuliaStats/StatsFuns.jl/blob/13e231a0a22e716426b73cb87ff3b8b24e33aaf1/src/distrs/norm.jl#L3

alyst · 2022-02-22T22:18:43Z

The polymorphic nature of the Distribution.jl ones is/was quite convenient.
If the ones from StatsFuns.jl are not exported, maybe it makes sense to restore zval/xval for Normal here?

devmotion · 2022-02-22T22:38:35Z

I don't understand, why would you you want them to be imported? They are not exported by StatsFuns because they are an internal implementation detail. So if you want to use them you could just call StatsFuns.xval etc., with the risk of them being removed in a future non-breaking release. Additionally, the version in StatsFuns is defined for real arguments whereas the ones in Distributions are defined for Distributions. But also in Distributions they are only internal functions and not part of the API - they may be removed at any time and they are only defined for a handful of functions.

alyst · 2022-02-22T23:00:39Z

I don't want StatsFuns.zval to be imported.
In my code, I was using the ones from Distributions.jl that have the signature zval(d::Distribution, x::Number) (that's what I mean by "polymorphic").
So I was wondering if it makes sense to restore zval(d::Normal, x::Number) here.
I understand that non-exported things can break at any time, I just wonder what is your recommendation to get the z-value for a given distribution (symmetric continuous univariate)?
For me, zval(d::Distribution, x::Number) was a convenient interface for that. Obviously, for some distributions it doesn't make sense to define zval().

As for the ones in StatsFuns.jl, since they are not exported, the name clash with Distributions.jl is not critical, but to be consistent with the other normal-related functions there, they could be renamed into normzval/normxval().

devmotion · 2022-02-22T23:56:02Z

Taking one step back, what exactly do you expect that zval(d::Distribution, x::Number) should return? In Distributions it is used as an auxiliary function and also for some asymmetric distributions in a way that allows us to simplify the logpdf etc. implementations. Many definitions are identical to zval(d, x) = (x - location(d)) / scale(d) but this is not satisfied in general.

alyst · 2022-02-23T00:33:09Z

Taking one step back, what exactly do you expect that zval(d::Distribution, x::Number) should return?
Many definitions are identical to zval(d, x) = (x - location(d)) / scale(d) but this is not satisfied in general.

That's what I expect (for the transformed distribution the mean is zero, the std is one), and that's why I specify that it might make sense to define it only for symmetric continuous univariate.

devmotion · 2022-02-23T00:37:55Z

In that case, can't you define and use this function myzval(d, x) = (x - location(d)) / scale(d) in your code? scale and location are part of the API, so this seems a clean and safe solution.

alyst · 2022-02-23T00:42:09Z

I can, I was just wondering if it could be implemented by the package, given that zval() is already defined for a dozen of distributions.

Use Julia implementations of normpdf etc. in StatsFuns

ef35e00

devmotion commented Jan 22, 2022

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

devmotion mentioned this pull request Jan 22, 2022

Make Distributions.jl GPU friendly #1067

Open

4 tasks

sethaxen reviewed Jan 22, 2022

View reviewed changes

src/univariate/continuous/normal.jl Outdated Show resolved Hide resolved

Project.toml Outdated Show resolved Hide resolved

Update Project.toml

7833c62

Skip tests on Julia < 1.3

bc2c60d

devmotion added 2 commits January 25, 2022 15:11

Simplify gradlogpdf

0c19af2

Fix LogNormal tests

529b907

devmotion requested a review from mschauer January 25, 2022 15:21

devmotion and others added 3 commits February 1, 2022 00:59

Require Julia >= 1.3 and SpecialFunctions >= 1.2

cd880c8

Merge branch 'master' into dw/normal_statsfuns

f8abd28

Revert changes

86113fd

sethaxen approved these changes Feb 2, 2022

View reviewed changes

mschauer merged commit 620a73a into master Feb 2, 2022

devmotion deleted the dw/normal_statsfuns branch February 2, 2022 14:38

devmotion mentioned this pull request Feb 9, 2022

Fix quantile_bisect with single point in bracketing interval #1505

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Julia implementations of `normpdf` etc. in StatsFuns #1487

Use Julia implementations of `normpdf` etc. in StatsFuns #1487

devmotion commented Jan 22, 2022

sethaxen left a comment

devmotion commented Jan 24, 2022

devmotion commented Jan 24, 2022

codecov-commenter commented Jan 25, 2022 •

edited

Loading

devmotion commented Feb 1, 2022 •

edited

Loading

sethaxen left a comment

devmotion commented Feb 2, 2022

alyst commented Feb 22, 2022

devmotion commented Feb 22, 2022

alyst commented Feb 22, 2022 •

edited

Loading

devmotion commented Feb 22, 2022

alyst commented Feb 22, 2022

devmotion commented Feb 22, 2022

alyst commented Feb 22, 2022 •

edited

Loading

devmotion commented Feb 22, 2022

alyst commented Feb 23, 2022

devmotion commented Feb 23, 2022

alyst commented Feb 23, 2022

Use Julia implementations of normpdf etc. in StatsFuns #1487

Use Julia implementations of normpdf etc. in StatsFuns #1487

Conversation

devmotion commented Jan 22, 2022

sethaxen left a comment

Choose a reason for hiding this comment

devmotion commented Jan 24, 2022

devmotion commented Jan 24, 2022

codecov-commenter commented Jan 25, 2022 • edited Loading

Codecov Report

devmotion commented Feb 1, 2022 • edited Loading

sethaxen left a comment

Choose a reason for hiding this comment

devmotion commented Feb 2, 2022

alyst commented Feb 22, 2022

devmotion commented Feb 22, 2022

alyst commented Feb 22, 2022 • edited Loading

devmotion commented Feb 22, 2022

alyst commented Feb 22, 2022

devmotion commented Feb 22, 2022

alyst commented Feb 22, 2022 • edited Loading

devmotion commented Feb 22, 2022

alyst commented Feb 23, 2022

devmotion commented Feb 23, 2022

alyst commented Feb 23, 2022

Use Julia implementations of `normpdf` etc. in StatsFuns #1487

Use Julia implementations of `normpdf` etc. in StatsFuns #1487

codecov-commenter commented Jan 25, 2022 •

edited

Loading

devmotion commented Feb 1, 2022 •

edited

Loading

alyst commented Feb 22, 2022 •

edited

Loading

alyst commented Feb 22, 2022 •

edited

Loading