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

getBigRandom() is incorrect #221

Open
FGasper opened this issue Dec 10, 2016 · 9 comments
Open

getBigRandom() is incorrect #221

FGasper opened this issue Dec 10, 2016 · 9 comments

Comments

@FGasper
Copy link

FGasper commented Dec 10, 2016

    this.getBigRandom = function (limit) {
	return new BigInteger(limit.bitLength(), rng)
	.mod(limit.subtract(BigInteger.ONE))
	.add(BigInteger.ONE)
	;
};

The above will unfairly privilege low numbers at the expense of high numbers. The initial BigInteger object is equally likely to be any number with the same bit length as “limit”. The modulo operation, then, makes for at least two separate ways by which the result of the operation can be a low number, while leaving only one path for the highest.

Example:

var r_int = Math.floor( 10 * Math.random() );
var r_int2 = r_int % 8;

r_int is equally likely to be any of the integers 0 to 9, inclusive; i.e., each integer has a 0.1 probability.

r_int2, however, has a .2 probability of being 0 and a .2 probability of being 1, while the probability of other possible r_int2 values is still 0.1.

@Ruffio
Copy link

Ruffio commented Jun 29, 2017

@FGasper Mayby I just don't understand your example, but with those two lines you will always get a number less than 10, right? So why do you write about getting greater than 10?

@FGasper
Copy link
Author

FGasper commented Jul 5, 2017

@Ruffio Sorry, that was a bad example before. I’ve updated it to be (hopefully) clearer.

@Ruffio
Copy link

Ruffio commented Jul 5, 2017

@FGasper do you have an idea of a 'correct' implementation of 'getBigRandom'? This is a little over my head... :-(

@FGasper
Copy link
Author

FGasper commented Jul 5, 2017

What is getBigRandom’s exact purpose? It seems to exclude 0 as a return value; is that by design?

A more-correct implementation would be to discard any random values that are greater than the maximum value we’d want to return.

Maybe something like:

    this.getBigRandom = function (limit) {
        var limit_minus_1 = limit.subtract(BigInteger.ONE);
	while (1) {
            var int = new BigInteger(limit.bitLength(), rng);
            if (int.lessThan( limit_minus_1 )) {   // not sure if this exists
                return int.add(BigInteger.ONE);
            }
	}
};

Sorry for the untested-ness … hopefully later I can flesh it out a bit more.

@Ruffio
Copy link

Ruffio commented Jul 25, 2017

@kjur is this bias of probability by intend or accidental? Should it be changed?

3 similar comments
@Ruffio
Copy link

Ruffio commented Jul 29, 2017

@kjur is this bias of probability by intend or accidental? Should it be changed?

@Ruffio
Copy link

Ruffio commented Aug 16, 2017

@kjur is this bias of probability by intend or accidental? Should it be changed?

@Ruffio
Copy link

Ruffio commented Aug 27, 2017

@kjur is this bias of probability by intend or accidental? Should it be changed?

@JerryUS2019
Copy link

@kjur what do you think?

tvogel added a commit to tvogel/jsrsasign that referenced this issue Nov 14, 2024
this replaces the previously remainder-based limiting of the random number
which caused bias toward small numbers and excluded zero altogether by
simple filtering as proposed frequently in
kjur#221
and because the performance in most cases is actually faster than in the
present implementation;

also, an adaptation of swiftlang/swift#39143 has
been considered but it performed significantly slower for large integers;
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

3 participants