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

[libc++] Adjust some of the [rand.dist] critical values that are too strict #88669

Merged
merged 2 commits into from
May 4, 2024

Conversation

MattStephanson
Copy link
Contributor

@MattStephanson MattStephanson commented Apr 14, 2024

Adjust some of the [rand.dist] critical values that are too strict

  • Most critical values are determined empirically by running each test 51
    times with a different PRNG seed and finding the smallest symmetric interval
    around the median that contains 90% of the sample means, variances, etc.

  • For the Kolmogorov-Smirnov tests, the alpha=0.1 critical value for large N
    is 1.224/sqrt(N).

  • For normally distributed variates, the sample kurtosis is distributed as
    Normal(0, 24/N). For N=1e5, this gives a 90% confidence interval of
    0+/-0.0255. For Binomial(40, 0.25), which is approximately normal, the
    kurtosis is -0.0167, so the relative 90% CI is large, on the order of
    0.0255/0.0167 = 153%. In most cases the distribution of the sample kurtosis
    isn't known analytically, but similarly large relative tolerances can be
    expected if the kurtosis is near zero.

@MattStephanson MattStephanson requested a review from a team as a code owner April 14, 2024 18:11
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 14, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 14, 2024

@llvm/pr-subscribers-libcxx

Author: Matt Stephanson (MattStephanson)

Changes

In the MSVC STL, a few of the eval.pass tests under rand.dist are marked XFAIL, but the failures mostly haven't been looked into in detail. I was investigating P0952R2, and it caused many more tests to fail, so I decided to look at the tolerances in these tests systematically. For context, most of these tests are comparing the sample mean, variance, skew, and kurtosis to the analytic values, although a few tests are using the Kolmogorov–Smirnov test.

The tests I ran and the results are described here. With the new critical values proposed in this PR, there aren't any tolerance-related failures using the MSVC STL and the new generate_canonical specification.

In the rand.dist.sample.plinear test, I also fixed a narrowing warning-treated-as-error that was causing a test failure for MSVC.


Patch is 51.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88669.diff

28 Files Affected:

  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval.pass.cpp (+4-4)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval_param.pass.cpp (+4-4)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.PR44847.pass.cpp (+1-1)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp (+6-6)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp (+5-5)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval.pass.cpp (+6-6)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval_param.pass.cpp (+2-2)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval.pass.cpp (+7-7)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval_param.pass.cpp (+5-5)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.cauchy/eval.pass.cpp (+3-3)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval.pass.cpp (+3-3)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval_param.pass.cpp (+3-3)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.lognormal/eval.pass.cpp (+9-9)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.lognormal/eval_param.pass.cpp (+9-9)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.t/eval.pass.cpp (+4-4)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.t/eval_param.pass.cpp (+4-4)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.exp/eval.pass.cpp (+3-3)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.exp/eval_param.pass.cpp (+1-1)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.extreme/eval.pass.cpp (+4-4)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.extreme/eval_param.pass.cpp (+4-4)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.gamma/eval.pass.cpp (+3-3)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.gamma/eval_param.pass.cpp (+3-3)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.poisson/eval.pass.cpp (+7-7)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.poisson/eval_param.pass.cpp (+7-7)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.discrete/eval.pass.cpp (+4-4)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.pconst/eval.pass.cpp (+2-2)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp (+18-18)
  • (modified) libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp (+3-3)
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval.pass.cpp
index 9a7af92931dd3e..568bf34f1ea437 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval.pass.cpp
@@ -63,8 +63,8 @@ int main(int, char**)
         double x_kurtosis = (6 * sqr(d.p()) - 6 * d.p() + 1)/x_var;
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
-        assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+        assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.05);
     }
     {
         typedef std::bernoulli_distribution D;
@@ -99,8 +99,8 @@ int main(int, char**)
         double x_kurtosis = (6 * sqr(d.p()) - 6 * d.p() + 1)/x_var;
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
-        assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+        assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.05);
     }
 
   return 0;
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval_param.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval_param.pass.cpp
index 5584a9d1644896..dfaa9f1c89f991 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval_param.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval_param.pass.cpp
@@ -65,8 +65,8 @@ int main(int, char**)
         double x_kurtosis = (6 * sqr(p.p()) - 6 * p.p() + 1)/x_var;
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
-        assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+        assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.05);
     }
     {
         typedef std::bernoulli_distribution D;
@@ -103,8 +103,8 @@ int main(int, char**)
         double x_kurtosis = (6 * sqr(p.p()) - 6 * p.p() + 1)/x_var;
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
-        assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+        assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.05);
     }
 
   return 0;
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.PR44847.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.PR44847.pass.cpp
index 572a59b7a1926a..549f3cee4d4a45 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.PR44847.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.PR44847.pass.cpp
@@ -163,7 +163,7 @@ int main(int, char**) {
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
     assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.04);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.06);
 
     return 0;
 }
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp
index dbdd0972411940..1670df40769101 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp
@@ -68,7 +68,7 @@ void test1() {
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
     assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.04);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.08);
 }
 
 template <class T>
@@ -109,8 +109,8 @@ void test2() {
     double x_kurtosis = (1-6*d.p()*(1-d.p())) / x_var;
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
-    assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+    assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.08);
 }
 
 template <class T>
@@ -151,8 +151,8 @@ void test3() {
     double x_kurtosis = (1-6*d.p()*(1-d.p())) / x_var;
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
-    assert(std::abs((skew - x_skew) / x_skew) < 0.03);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.3);
+    assert(std::abs((skew - x_skew) / x_skew) < 0.07);
+    assert(std::abs(kurtosis - x_kurtosis) < 2.0);
 }
 
 template <class T>
@@ -292,7 +292,7 @@ void test6() {
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
     assert(std::abs(skew - x_skew) < 0.02);
-    assert(std::abs(kurtosis - x_kurtosis) < 0.01);
+    assert(std::abs(kurtosis - x_kurtosis) < 0.03);
 }
 
 template <class T>
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp
index 78d6aedde73a5d..adbcb78d10f4f2 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp
@@ -72,7 +72,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.04);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.08);
     }
     {
         typedef std::binomial_distribution<> D;
@@ -113,8 +113,8 @@ int main(int, char**)
         double x_kurtosis = (1-6*p.p()*(1-p.p())) / x_var;
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
-        assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.08);
     }
     {
         typedef std::binomial_distribution<> D;
@@ -155,8 +155,8 @@ int main(int, char**)
         double x_kurtosis = (1-6*p.p()*(1-p.p())) / x_var;
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
-        assert(std::abs((skew - x_skew) / x_skew) < 0.04);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.3);
+        assert(std::abs((skew - x_skew) / x_skew) < 0.07);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 2.0);
     }
 
   return 0;
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval.pass.cpp
index 440334ed3488ae..0cdb7fa6312caf 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval.pass.cpp
@@ -77,7 +77,7 @@ void test1() {
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
     assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
 }
 
 template <class T>
@@ -161,7 +161,7 @@ void test3() {
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
     assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
 }
 
 template <class T>
@@ -203,7 +203,7 @@ void test4() {
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
     assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
 }
 
 template <class T>
@@ -245,7 +245,7 @@ void test5() {
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
     assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
 }
 
 template <class T>
@@ -284,8 +284,8 @@ void test6() {
     double x_var = x_mean / d.p();
     double x_skew = (2 - d.p()) / std::sqrt((1 - d.p()));
     double x_kurtosis = 6 + sqr(d.p()) / (1 - d.p());
-    assert(std::abs((mean - x_mean) / x_mean) < 0.01);
-    assert(std::abs((var - x_var) / x_var) < 0.01);
+    assert(std::abs((mean - x_mean) / x_mean) < 0.02);
+    assert(std::abs((var - x_var) / x_var) < 0.02);
     assert(std::abs((skew - x_skew) / x_skew) < 0.01);
     assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
 }
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval_param.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval_param.pass.cpp
index 16cb7fb0a45f98..16a5bd4c7a33e7 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval_param.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval_param.pass.cpp
@@ -72,7 +72,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
     }
     {
         typedef std::geometric_distribution<> D;
@@ -156,7 +156,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
     }
 
   return 0;
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval.pass.cpp
index d0f6fbf0a1203f..be08361fc27e21 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval.pass.cpp
@@ -74,7 +74,7 @@ void test1() {
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
     assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.02);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
 }
 
 template <class T>
@@ -115,8 +115,8 @@ void test2() {
     double x_kurtosis = 6. / d.k() + sqr(d.p()) / (d.k() * (1 - d.p()));
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
-    assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+    assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.1);
 }
 
 template <class T>
@@ -157,8 +157,8 @@ void test3() {
     double x_kurtosis = 6. / d.k() + sqr(d.p()) / (d.k() * (1 - d.p()));
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
-    assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
+    assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.08);
 }
 
 template <class T>
@@ -243,8 +243,8 @@ void test5() {
     double x_kurtosis = 6. / d.k() + sqr(d.p()) / (d.k() * (1 - d.p()));
     assert(std::abs((mean - x_mean) / x_mean) < 0.01);
     assert(std::abs((var - x_var) / x_var) < 0.01);
-    assert(std::abs((skew - x_skew) / x_skew) < 0.04);
-    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.05);
+    assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+    assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.3);
 }
 
 template <class T>
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval_param.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval_param.pass.cpp
index 0b03982a737e57..26bc83382f6850 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval_param.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval_param.pass.cpp
@@ -72,7 +72,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
     }
     {
         typedef std::negative_binomial_distribution<> D;
@@ -113,8 +113,8 @@ int main(int, char**)
         double x_kurtosis = 6. / p.k() + sqr(p.p()) / (p.k() * (1 - p.p()));
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
-        assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.1);
     }
     {
         typedef std::negative_binomial_distribution<> D;
@@ -155,8 +155,8 @@ int main(int, char**)
         double x_kurtosis = 6. / p.k() + sqr(p.p()) / (p.k() * (1 - p.p()));
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
-        assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
+        assert(std::abs((skew - x_skew) / x_skew) < 0.02);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.08);
     }
 
   return 0;
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.cauchy/eval.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.cauchy/eval.pass.cpp
index abc0cc531a117e..98f32d00f88381 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.cauchy/eval.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.cauchy/eval.pass.cpp
@@ -45,7 +45,7 @@ int main(int, char**)
             u.push_back(d(g));
         std::sort(u.begin(), u.end());
         for (int i = 0; i < N; ++i)
-            assert(std::abs(f(u[i], a, b) - double(i)/N) < .001);
+            assert(std::abs(f(u[i], a, b) - double(i)/N) < .0013);
     }
     {
         typedef std::cauchy_distribution<> D;
@@ -60,7 +60,7 @@ int main(int, char**)
             u.push_back(d(g));
         std::sort(u.begin(), u.end());
         for (int i = 0; i < N; ++i)
-            assert(std::abs(f(u[i], a, b) - double(i)/N) < .001);
+            assert(std::abs(f(u[i], a, b) - double(i)/N) < .0013);
     }
     {
         typedef std::cauchy_distribution<> D;
@@ -75,7 +75,7 @@ int main(int, char**)
             u.push_back(d(g));
         std::sort(u.begin(), u.end());
         for (int i = 0; i < N; ++i)
-            assert(std::abs(f(u[i], a, b) - double(i)/N) < .001);
+            assert(std::abs(f(u[i], a, b) - double(i)/N) < .0013);
     }
 
   return 0;
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval.pass.cpp
index 2a8dfd31aa08eb..559034b2a0ec10 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval.pass.cpp
@@ -70,7 +70,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.04);
     }
     {
         typedef std::chi_squared_distribution<> D;
@@ -109,7 +109,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
     }
     {
         typedef std::chi_squared_distribution<> D;
@@ -148,7 +148,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
     }
 
   return 0;
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval_param.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval_param.pass.cpp
index 52864739c9b3cc..74454f29626724 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval_param.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval_param.pass.cpp
@@ -72,7 +72,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
     }
     {
         typedef std::chi_squared_distribution<> D;
@@ -113,7 +113,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.03);
     }
     {
         typedef std::chi_squared_distribution<> D;
@@ -154,7 +154,7 @@ int main(int, char**)
         assert(std::abs((mean - x_mean) / x_mean) < 0.01);
         assert(std::abs((var - x_var) / x_var) < 0.01);
         assert(std::abs((skew - x_skew) / x_skew) < 0.01);
-        assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.01);
+        assert(std::abs((kurtosis - x_kurtosis) / x...
[truncated]

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Can you update the commit message how you determined the new values; I guess empirical, but it would be good to know for certain.

I noticed the same when I worked on P0952R2. (That patch is still WIP and I got lower on my priority list.)

@@ -151,8 +151,8 @@ void test3() {
double x_kurtosis = (1-6*d.p()*(1-d.p())) / x_var;
assert(std::abs((mean - x_mean) / x_mean) < 0.01);
assert(std::abs((var - x_var) / x_var) < 0.01);
assert(std::abs((skew - x_skew) / x_skew) < 0.03);
assert(std::abs((kurtosis - x_kurtosis) / x_kurtosis) < 0.3);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I was debating changing it to an absolute test because the expected kurtosis is close to zero (-0.017). 2.0 should be the relative bound.

For reference, if you approximate the binomial distribution as normal, you would expect the sample kurtosis to have a standard deviation of $\sqrt{24/N} \approx 0.0155$. That gives a relative 90% confidence interval of $\sqrt{2}\mathrm{erf}^{-1}(0.9)\cdot 0.0155 / 0.017 \approx 1.5$, so I don't think the empirically observed value of 2.0 is unreasonable.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add something along these lines in the commit message. That give future readers more information why these changes are valid.

Copy link
Member

Choose a reason for hiding this comment

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

@MattStephanson can you update the commit message? Then the patch is good to land. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mordante I did update it the last time I force-pushed 788fc37. If it's still missing something, could you please be more specific? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I wasn't clear.
When we land this patch the commits will be squashed and merged into one commit. Then we only have the commit message of the review. So what I meant was to edit the top message with the additional information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, okay. I think I have it now?

Copy link
Member

Choose a reason for hiding this comment

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

Yes great! Thanks. I'll land the patch.

@EricWF
Copy link
Member

EricWF commented Apr 15, 2024

Is in not better to have smaller values on this?

If so, we may want to keep some testing to ensure our implementation doesn't slip?
But otherwise, this change seems reasonable.

@MattStephanson
Copy link
Contributor Author

Is in not better to have smaller values on this?

Not exactly. The statistics of a sample (mean, variance, etc.) are themselves random variables with some distribution. So except in degenerate cases, there's always some chance that a correct generator will give an out-of-tolerance result. There's also a tradeoff because if you want a smaller tolerance, you need more samples to justify rejecting the null hypothesis that the sample comes from the desired distribution. Even these larger tolerances that I'm proposing only approximate a 90% confidence interval, so if the underlying U[0,1) engine was truly random, you would still expect these tests to fail 10% of the time even if the distribution generator was completely correct.

Copy link

github-actions bot commented Apr 18, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 5927492e8e68c1ee5a0d84cf6c402a900aac4a3c 7f117556001c9d205af692a5ef4d6fa0e432a0e1 -- libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bernoulli/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.PR44847.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.geo/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.cauchy/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.chisq/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.lognormal/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.lognormal/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.t/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.norm/rand.dist.norm.t/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.exp/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.exp/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.extreme/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.extreme/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.gamma/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.gamma/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.poisson/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.pois/rand.dist.pois.poisson/eval_param.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.discrete/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.pconst/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp
View the diff from clang-format here.
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp
index 8aad0b8e4a..06f57dc69d 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp
@@ -85,7 +85,7 @@ test1()
         bk = b[k];
         c  = (b[k + 1] * p[k] - b[k] * p[k + 1]) / (b[k + 1] - b[k]);
         kp = k;
-        }
+      }
       assert(std::abs(f(u[i], a, m, bk, c) - double(i) / N) < .0013);
     }
 }
@@ -136,7 +136,7 @@ test2()
         bk = b[k];
         c  = (b[k + 1] * p[k] - b[k] * p[k + 1]) / (b[k + 1] - b[k]);
         kp = k;
-        }
+      }
       assert(std::abs(f(u[i], a, m, bk, c) - double(i) / N) < .0013);
     }
 }
@@ -187,7 +187,7 @@ test3()
         bk = b[k];
         c  = (b[k + 1] * p[k] - b[k] * p[k + 1]) / (b[k + 1] - b[k]);
         kp = k;
-        }
+      }
       assert(std::abs(f(u[i], a, m, bk, c) - double(i) / N) < .0013);
     }
 }
@@ -239,7 +239,7 @@ test4()
         bk = b[k];
         c  = (b[k + 1] * p[k] - b[k] * p[k + 1]) / (b[k + 1] - b[k]);
         kp = k;
-        }
+      }
       assert(std::abs(f(u[i], a, m, bk, c) - double(i) / N) < .0013);
     }
 }
@@ -292,7 +292,7 @@ test5()
         bk = b[k];
         c  = (b[k + 1] * p[k] - b[k] * p[k + 1]) / (b[k + 1] - b[k]);
         kp = k;
-        }
+      }
       assert(std::abs(f(u[i], a, m, bk, c) - double(i) / N) < .0013);
     }
 }
@@ -343,7 +343,7 @@ test6()
         bk = b[k];
         c  = (b[k + 1] * p[k] - b[k] * p[k + 1]) / (b[k + 1] - b[k]);
         kp = k;
-        }
+      }
       assert(std::abs(f(u[i], a, m, bk, c) - double(i) / N) < .0013);
     }
 }
diff --git a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp
index 4601c36019..b847678aae 100644
--- a/libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp
@@ -87,7 +87,7 @@ int main(int, char**)
             bk = b[k];
             c  = (b[k + 1] * p[k] - b[k] * p[k + 1]) / (b[k + 1] - b[k]);
             kp = k;
-            }
+          }
           assert(std::abs(f(u[i], a, m, bk, c) - double(i) / N) < .0013);
         }
     }

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Is in not better to have smaller values on this?

I kind of agree, however these are statistical tests and there is no motivation why these tolerances were picked. For example 45a9997

I noticed the tolerances not to work when implementing a new paper.

If so, we may want to keep some testing to ensure our implementation doesn't slip? But otherwise, this change seems reasonable.

I think we could add a test where we test some specific expected values. To me this is outside the scope of this patch; unless @MattStephanson likes to do it.

I'm happy with this patch after addressing my last comment. Please give @EricWF a few days to respond before landing this patch.

 - Most critical values are determined empirically by running each test 51
   times with a different PRNG seed and finding the smallest symmetric interval
   around the median that contains 90% of the sample means, variances, etc.

 - For the Kolmogorov-Smirnov tests, the alpha=0.1 critical value for large N
   is 1.224/sqrt(N).

 - For normally distributed variates, the sample kurtosis is distributed as
   Normal(0, 24/N). For N=1e5, this gives a 90% confidence interval of
   0+/-0.0255. For Binomial(40, 0.25), which is approximately normal, the
   kurtosis is -0.0167, so the relative 90% CI is large, on the order of
   0.0255/0.0167 = 153%. In most cases the distribution of the sample kurtosis
   isn't known analytically, but similarly large relative tolerances can be
   expected if the kurtosis is near zero.
@mordante
Copy link
Member

mordante commented May 3, 2024

I noticed your email is hidden, could you make that public?

@MattStephanson
Copy link
Contributor Author

I noticed your email is hidden, could you make that public?

Okay.

@mordante mordante merged commit 76aa042 into llvm:main May 4, 2024
50 of 51 checks passed
Copy link

github-actions bot commented May 4, 2024

@MattStephanson Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants