Re: RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions [v2]
On Wed, 1 Dec 2021 17:38:54 GMT, Jim Laskey wrote: >> The modified ziggurat algorithm is not correctly implemented in >> `java.base/jdk/internal/util/random/RandomSupport.java`. >> >> Create a histogram of a million samples using 2000 uniform bins with the >> following range: >> Exponential range from 0 to 12. Gaussian range from -8 to 8. >> >> This does not pass a Chi-square test. If you look at the histogram it is >> obviously not showing the shape of the PDF for these distributions. Look >> closely at the range around zero (e.g. +/- 0.5). > > Jim Laskey has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains two additional commits since > the last revision: > > - Merge branch 'master' into 8273056 > - 8273056 - java.util.random does not correctly sample exponential or > Gaussian distributions Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6353
Re: RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions [v2]
> The modified ziggurat algorithm is not correctly implemented in > `java.base/jdk/internal/util/random/RandomSupport.java`. > > Create a histogram of a million samples using 2000 uniform bins with the > following range: > Exponential range from 0 to 12. Gaussian range from -8 to 8. > > This does not pass a Chi-square test. If you look at the histogram it is > obviously not showing the shape of the PDF for these distributions. Look > closely at the range around zero (e.g. +/- 0.5). Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into 8273056 - 8273056 - java.util.random does not correctly sample exponential or Gaussian distributions - Changes: - all: https://git.openjdk.java.net/jdk/pull/6353/files - new: https://git.openjdk.java.net/jdk/pull/6353/files/b10c4793..b6679479 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6353=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6353=00-01 Stats: 69228 lines in 1218 files changed: 46725 ins; 12712 del; 9791 mod Patch: https://git.openjdk.java.net/jdk/pull/6353.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6353/head:pull/6353 PR: https://git.openjdk.java.net/jdk/pull/6353
Re: RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions
On Wed, 1 Dec 2021 14:21:51 GMT, Jim Laskey wrote: > > Should there be a regression test here? > > I can ask the submitter to provide a new test based on his testing (new JBS > issue.) The complexity of the test is beyond in house expertise. Okay. - PR: https://git.openjdk.java.net/jdk/pull/6353
Re: RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions
On Thu, 11 Nov 2021 13:59:51 GMT, Jim Laskey wrote: > The modified ziggurat algorithm is not correctly implemented in > `java.base/jdk/internal/util/random/RandomSupport.java`. > > Create a histogram of a million samples using 2000 uniform bins with the > following range: > Exponential range from 0 to 12. Gaussian range from -8 to 8. > > This does not pass a Chi-square test. If you look at the histogram it is > obviously not showing the shape of the PDF for these distributions. Look > closely at the range around zero (e.g. +/- 0.5). Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6353
Re: RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions
On Tue, 30 Nov 2021 22:23:01 GMT, Joe Darcy wrote: > Should there be a regression test here? I can ask the submitter to provide a new test based on his testing (new JBS issue.) The complexity of the test is beyond in house expertise. - PR: https://git.openjdk.java.net/jdk/pull/6353
Re: RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions
On Thu, 11 Nov 2021 13:59:51 GMT, Jim Laskey wrote: > The modified ziggurat algorithm is not correctly implemented in > `java.base/jdk/internal/util/random/RandomSupport.java`. > > Create a histogram of a million samples using 2000 uniform bins with the > following range: > Exponential range from 0 to 12. Gaussian range from -8 to 8. > > This does not pass a Chi-square test. If you look at the histogram it is > obviously not showing the shape of the PDF for these distributions. Look > closely at the range around zero (e.g. +/- 0.5). Should there be a regression test here? - PR: https://git.openjdk.java.net/jdk/pull/6353
Re: RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions
On Thu, 11 Nov 2021 13:59:51 GMT, Jim Laskey wrote:
> The modified ziggurat algorithm is not correctly implemented in
> `java.base/jdk/internal/util/random/RandomSupport.java`.
>
> Create a histogram of a million samples using 2000 uniform bins with the
> following range:
> Exponential range from 0 to 12. Gaussian range from -8 to 8.
>
> This does not pass a Chi-square test. If you look at the histogram it is
> obviously not showing the shape of the PDF for these distributions. Look
> closely at the range around zero (e.g. +/- 0.5).
Looks fine.
src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
1191:
> 1189: // At this point, the high-order bits of U1 have not
> been used yet,
> 1190: // but we need the value in U1 to be positive.
> 1191: for (U1 = (U1 >>> 1);; U1 = (rng.nextLong() >>> 1)) {
A minor thing, but I would probably write `U1 >>>= 1` instead of `U1 = (U1 >>>
1)`.
-
Marked as reviewed by bpb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6353
Re: RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions
On Thu, 11 Nov 2021 13:59:51 GMT, Jim Laskey wrote: > The modified ziggurat algorithm is not correctly implemented in > `java.base/jdk/internal/util/random/RandomSupport.java`. > > Create a histogram of a million samples using 2000 uniform bins with the > following range: > Exponential range from 0 to 12. Gaussian range from -8 to 8. > > This does not pass a Chi-square test. If you look at the histogram it is > obviously not showing the shape of the PDF for these distributions. Look > closely at the range around zero (e.g. +/- 0.5). Still waiting for a review on these changes. - PR: https://git.openjdk.java.net/jdk/pull/6353
RFR: JDK-8273056 java.util.random does not correctly sample exponential or Gaussian distributions
The modified ziggurat algorithm is not correctly implemented in `java.base/jdk/internal/util/random/RandomSupport.java`. Create a histogram of a million samples using 2000 uniform bins with the following range: Exponential range from 0 to 12. Gaussian range from -8 to 8. This does not pass a Chi-square test. If you look at the histogram it is obviously not showing the shape of the PDF for these distributions. Look closely at the range around zero (e.g. +/- 0.5). - Commit messages: - 8273056 - java.util.random does not correctly sample exponential or Gaussian distributions Changes: https://git.openjdk.java.net/jdk/pull/6353/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6353=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273056 Stats: 15 lines in 1 file changed: 6 ins; 4 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6353.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6353/head:pull/6353 PR: https://git.openjdk.java.net/jdk/pull/6353
