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

nextInt is not uniform #3

Closed
Quijx opened this issue Mar 13, 2022 · 1 comment
Closed

nextInt is not uniform #3

Quijx opened this issue Mar 13, 2022 · 1 comment

Comments

@Quijx
Copy link

Quijx commented Mar 13, 2022

nextInt(n) does not generate uniform random numbers for larger n.

The issue is with the following line:

for (int u = r; u - (r = u % max) + m < 0; u = nextRaw32()) {}

If I'm not mistaken the line should be for (int u = r; u - (r = u % max) + max > (1 << 32); u = nextRaw32()) {}. Not sure what the point of m = max-1 is, but i don't think its needed.

Since u % max is always <= u, u-(u%max) is never negative. And since m is also never negative, u-(r=u%max)+m will never be less than 0. So essentially only nextRaw32() % max will every be returned. This is approximately uniform for small m but for larger m, it is not. This can easily experimentally be verified.

The following program prints lower: 6665204, upper: 3334796 even though there should be approximately an equal amout of numers below the midpoint as above.

import 'package:xrandom/xrandom.dart';

void main() {
  final random = Xrandom(42);

  const mid = (1 << 32) ~/ 3;
  const max = mid * 2;
  var lower = 0;
  var upper = 0;
  for (var i = 0; i < 10000000; i++) {
    if (random.nextInt(max) < mid) {
      lower++;
    } else {
      upper++;
    }
  }
  print('lower: $lower, upper: $upper');
}
@rtmigo
Copy link
Owner

rtmigo commented Mar 14, 2022

Thanks for pointing this out. Indeed there was a problem.

It seemed to happen for all max exceeding the range of a signed int32 (for max from 1<<31 to 1<<32).

As a quick fix, I patched this method with the standard algorithm from the Dart SDK. And also added tests with a uniformity check for a given range of max.

@Quijx Quijx closed this as completed Mar 14, 2022
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

No branches or pull requests

2 participants