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

big_random function: mistake in description and better implementation suggestion #48

Open
B1Z0N opened this issue Aug 29, 2019 · 3 comments · May be fixed by #49
Open

big_random function: mistake in description and better implementation suggestion #48

B1Z0N opened this issue Aug 29, 2019 · 3 comments · May be fixed by #49
Labels
enhancement New feature or request functions: random Random functions

Comments

@B1Z0N
Copy link

B1Z0N commented Aug 29, 2019

Error in description

I assume that author has written right implementation and forgot about description and not otherwise. So he meant to generate positive BigInts.
Then this:

Returns a random BigInt with a specific number of digits

should be changed to

Returns a random positive BigInt with a specific number of digits

This should be fixed in include/functions/random.hpp and README.md.

Better implementation

  1. std::random_device is used to generate random digits, but it is not intended to be used that way for general purposes(read this). So I suggest to use the modern way - std::mt19937. Properly seeded it has 2^19937-1 potential states(that's a lot).

  2. When we are using % operation to achieve uniform integer distribution, that's not the right way, because some numbers are more likely to come up(read this for example). So let's use the right and modern way - std::uniform_int_distribution to generate

    • digits in BigInt number.
    • random length of BigInt number
@Prhmma
Copy link

Prhmma commented Oct 20, 2019

this looks nice,
i would like to work on that

@B1Z0N
Copy link
Author

B1Z0N commented Oct 25, 2019

let's start, add your suggestions or some code please

anyway, we can't do much without @faheel's attention here

@faheel
Copy link
Owner

faheel commented Oct 5, 2020

@B1Z0N Sorry for not taking a look at this for almost a year! Your idea seems good. I'll review your PR when I get more time.

And I really don't remember whether the initial intention was to only create positive BigInts or did I forget about negative numbers 😄

@faheel faheel added enhancement New feature or request functions: random Random functions labels Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request functions: random Random functions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants