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

Adds PCG32 random generator, makes it default #4536

Merged
merged 1 commit into from
Jun 29, 2017
Merged

Adds PCG32 random generator, makes it default #4536

merged 1 commit into from
Jun 29, 2017

Conversation

konovod
Copy link
Contributor

@konovod konovod commented Jun 9, 2017

Implementation of #4493
I've changed Random::DEFAULT to PCG32 as it looks pointless to add better (for most use cases) generator without making it default.

@@ -0,0 +1,80 @@
# This is a Crystal conversion of basic C PCG implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest naming this file with full algo name, like pcg32.cr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The naming is because there also exists PCG64 and some variations with different seeding that share most of code, so i thought that they could be added to this file later. On the other hand, right now it doesn't make sense so i'll change it.

@ysbaddaden
Copy link
Contributor

I'm totally fine with this, obviously. Performance and quality is greatly improved.

I'd like the opinion of others on this.

@asterite
Copy link
Member

@konovod I like this, thank you!

I don't know much about random number generators, but the comments in #4493 and links to papers seem to be good.

If we merge this, I'd like to move other PRNG to shards. I think it's better to just have a single good random number generator in the standard library (and a good secure one too), but that's it, so users don't feel overwhelmed. If they need a specific one for their characteristics they can find an external shard.

@RX14
Copy link
Contributor

RX14 commented Jun 10, 2017

@asterite presumably they would live on the crystal-lang org so that users knew they had good support?

@asterite
Copy link
Member

I'm not sure. I don't want to maintain that.

@konovod
Copy link
Contributor Author

konovod commented Jun 10, 2017

i've added Mersenne Twister and ISAAC to prngs as it's, well, just copying a files 😆
Can't guarantee good support of course.

@mverzilli
Copy link

@RX14 i think that's the best way to go. @asterite, presumably the maintenance burden shouldn't be worse than now, and having it in a separate repo might make it easier for us to grant write permissions to someone in the community who's interested in monitoring it.

@RX14
Copy link
Contributor

RX14 commented Jun 10, 2017

Random generator implementations strike me as pretty low maintenance. They can't really get more features and bugs are likely rare as we can compare directly vs existing implementations. Due to their nature it's unlikely that corner cases exist.

@asterite
Copy link
Member

Well, there have been a bunch of PRs related to PRNG, so I'd like to disagree on that 😸

In any case, we can leave the implementations in the std... but at least I won't maintain them.

@RX14
Copy link
Contributor

RX14 commented Jun 10, 2017

@asterite Have there? I only recall one or two adding a new PRNG and none modifying the PRNG code.

@sdogruyol
Copy link
Member

sdogruyol commented Jun 29, 2017

@asterite you said this is pretty good looking, how about we merge this? 😄

@mverzilli mverzilli added this to the Next milestone Jun 29, 2017
@mverzilli mverzilli merged commit 76b4f2d into crystal-lang:master Jun 29, 2017
@mverzilli
Copy link

Thank you @konovod!

@sdogruyol
Copy link
Member

@mverzilli thanks 👍 @konovod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants