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

Move rand_pcg to rngs repository #1139

Closed
vks opened this issue Jun 16, 2021 · 9 comments
Closed

Move rand_pcg to rngs repository #1139

vks opened this issue Jun 16, 2021 · 9 comments

Comments

@vks
Copy link
Collaborator

vks commented Jun 16, 2021

rand_pcg is no longer used for SmallRng, therefore it may be moved to the rust-random/rngs repository. This does not affect the API, only the repository and the issue tracker.

This will result in a bit of churn, but I think it's good to move unused code from the rust-random/rand repository.

We might also want to move rand_hc, which is currently only relevant for emscripten targets, because they lack u128 support. Maybe this was fixed in the meanwhile?

@dhardy
Copy link
Member

dhardy commented Jun 16, 2021

I agree that rand_pcg should move. A little searching the web doesn't show me any progress on making emscripten support 128-bit integers. I think rand_hc should therefore stay here.

@dhardy
Copy link
Member

dhardy commented Jul 8, 2021

Just looked at moving rand_pcg and realised it's still used by quite a few tests in rand and rand_distr. So, we have to choose:

  • depending on out-of-tree RNGs is problematic when bumping the rand_core version, which isn't 1.0 yet
  • PCG is not much code; we could just put an implementation in each library (under #[cfg(test)])
  • rand::rngs::SmallRng is not suitable for our value stability tests
  • we could just leave rand_pcg where it is (in the source tree)

@vks
Copy link
Collaborator Author

vks commented Jul 8, 2021

I see. I think our best option is to embed the PCG implementation, or leave it as it is.

@vks
Copy link
Collaborator Author

vks commented Jul 8, 2021

About emscripten: It looks like they fixed 128-bit integer support according to emscripten-core/emscripten-fastcomp#169?

@dhardy
Copy link
Member

dhardy commented Jul 8, 2021

I see. I think our best option is to embed the PCG implementation, or leave it as it is.

Yes. Either is fine IMO.

About emscripten ...

So we can test using rand_chacha now? Except, we don't have any CI tests using emscripten since #874, so we need some way of testing whether removing the emscripten special casing in Cargo.toml is okay. I wonder if someone like @est31 could help with that?

@vks
Copy link
Collaborator Author

vks commented Sep 15, 2021

Now that we no longer have a special case for emscripten, I agree that we have the following options:

  1. Inline PCG and remove rand_pcg.
  2. Replace use of rand_pcg with rand_chacha.
  3. Keep rand_pcg and close this issue.

Option 2 implies significantly more churn than the others, but I don't see any other problems.

@dhardy
Copy link
Member

dhardy commented Sep 16, 2021

Option (3) is the least effort. Is there a good reason not to leave it here?

@newpavlov
Copy link
Member

I think we can follow (3) for now and open a low-priority issue for porting tests from rand_pcg to rand_chacha.

@vks
Copy link
Collaborator Author

vks commented Sep 16, 2021

Thanks for the input, I opened #1185 in favor of this issue.

@vks vks closed this as completed Sep 16, 2021
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