Random number generation

[phykas]

Hi guys, First, enjoying your library so far, and want to thank you for investing your time into porting this great library to js. I do have one question, and it's about this line:

https://github.com/HubSpot/PlanOut.js/blob/master/es6/ops/randomBase.js#L8 hashCalculation(hash) { return parseInt(hash.substr(0, 13), 16); }

Are you sure that should be 13, because I'm doing some C# library and using yours as a reference. When I use 15 instead of 13, I get normal probabilities also normalizing the numbers works fine as well. But with 13 this part of the code is not working for me: var zeroToOne = this.zeroToOneCalculation(appendedUnit); return zeroToOne * (maxVal - minVal) + minVal; Check out the Java port is also using 15 char substring: https://github.com/Glassdoor/planout4j/blob/master/core/src/main/java/com/glassdoor/planout4j/planout/ops/random/PlanOutOpRandom.java#L73

Also the original Facebook library is using 15: https://github.com/facebook/planout/blob/master/python/planout/ops/random.py#L30

[phykas]

"The underlying randomization ops in the JavaScript implementation return different results for efficiency reasons." This is probably the reason? :)

[mattrheault]

Yeah this is definitely for efficiency reasons and possibly JS's MAX_SAFE_INTEGER, though I can't remember whether that played a factor. @rawls238 might remember better on that.

Either way, the JS port of planout is not core compatible by default with it's sibling implementations. If you've gotta have core compatibility, then you'll need to use the core_compatible bundle which has bignumber.js as a dependency and performs random operations much less efficiently.

We split the core compatible bundle out because most people don't need their random operations to match the other implementations, so efficiency took precedence.

[rawls238]

Sorry I meant to respond to this. It has to do with JS's MAX_SAFE_INTEGER as @mattrheault mentioned. Ideally we could just use 15 here and so many headaches would be saved, but we can't so we have to resort to using bignumber.js to get compatibility which results in efficiency issues relative to not using the library. In your C# library you should use 15.

[eytan]

+1.

On Mon, Apr 24, 2017 at 4:15 PM, Guy Aridor notifications@github.com wrote:

Sorry I meant to respond to this. It has to do with JS's MAX_SAFE_INTEGER as @mattrheault https://github.com/mattrheault mentioned. Ideally we could just use 15 here and so many headaches would be saved, but we can't so we have to resort to using bignumber.js to get compatibility which results in efficiency issues relative to not using the library. In your C# library you should use 15.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/HubSpot/PlanOut.js/issues/61#issuecomment-296847661, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFt8Q_PQxk7i8wXFRk__WE28wNMArXhks5rzSz2gaJpZM4NFcNQ .

[phykas]

Thank you very much for your answer, I'll use 15 in C# then... :)