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

Doc and policies #544

Merged
merged 5 commits into from Jul 23, 2018
Merged

Doc and policies #544

merged 5 commits into from Jul 23, 2018

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jul 9, 2018

Addresses #408, #538 and migrates wiki Policies under version control.

The intention is that we may add more types of documentation under doc and potentially turn this into a book. It is also to bring documentation currently on the wiki under version control to allow proper review.

We can improve the layout of this documentation later (perhaps one new policies document, perhaps multiple). Also we should incorporate or link CONTRIBUTING.md and the changelog. But I don't want to focus on presentation in this PR...

... however I do want review of the content added here, in particular of the project policies.

This document leaves scope and value-stability partially undefined, but hopefully is precise enough for now. Hopefully the policy on new PRNGs is also clearer than the current wiki.

So, review please?

@dhardy dhardy added E-question Participation: opinions wanted C-docs Documentation D-review Do: needs review labels Jul 9, 2018
doc/README.md Outdated
Rand does **make use of `unsafe`**, both for performance and out of necessity.
We consider this acceptable so long as correctness is easy to verify.
In order to make this as simple as possible,
we prefer that all parameters affecting safety of `unsafe` blocks as checked or
Copy link
Collaborator

Choose a reason for hiding this comment

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

blocks are checked?

@TheIronBorn
Copy link
Collaborator

Perhaps we should mention PRNG portability/hardware support?
i.e. 128-bit multiplications on 32-bit architecture, MULX/MULQ/CRC instructions, AES-NI

@dhardy
Copy link
Member Author

dhardy commented Jul 10, 2018

@TheIronBorn is it necessary to discuss instruction sets here? Perhaps there could be more on portability though. Would you like to contribute?

doc/README.md Outdated
a scientific article or web page) introducing the new algorithm and
discussing its utility, strengths and weaknesses
- review of statistical quality and any special features by third parties
- good performance in automated test suites like PractRand, TestU01 and
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is meant by "good performance"? Is this intentionally vague, so it has to be decided on a case-by-case basis?

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you propose then?

I mean obviously "useful performance" depends on what other PRNGs are available with the required features. It seems hard to be specific here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also depends on the test. In general, it is possible to design a test for any non-CSPRNG where it fails, otherwise it would be a CSPRNG. So for example a strict "no test failure" policy is too simplistic. I think it should be decided on a case-by-case basis.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are aware that not everything is provable, right? For example the ISAAC generator has not been proven either secure or insecure. So yes, one generally has to guess a bit on a case-by-case basis.

Copy link
Contributor

Choose a reason for hiding this comment

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

BigCrush is part of TestU01, some maybe this should just say "good performance in automated test suites like PractRand and BigCrush"

I also depends on the test. In general, it is possible to design a test for any non-CSPRNG where it fails, otherwise it would be a CSPRNG.

That is a point, we can't say that the PRNG should pass any test one can create. PractRand and BigCrush are not made to pick on any specific PRNG algorithm, but to evaluate how good they are for practical scenarios. The results on these test should definitely be a big factor for consideration.

This requirement would make it hard for your favourite Xoroshiro to become part of Rand, and keep most of the old, worse, designs out the door.

Copy link
Collaborator

Choose a reason for hiding this comment

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

PractRand and BigCrush are not made to pick on any specific PRNG algorithm, but to evaluate how good they are for practical scenarios.

This is not accurate. For example, the binary rank test is specifically designed with shift-register generators in mind (see the original Marsaglia paper).

This requirement would make it hard for your favourite Xoroshiro to become part of Rand, and keep most of the old, worse, designs out the door.

I still think the binary rank failures don't matter, but "my favorite" xoshiro does not have these issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we please stop quibbling about xo*shiro and binary rank failures? The RNGs are perfectly good enough for inclusion in some Rand sub-crate or external crate, and which we use for SmallRng is another matter.

This policy is intended as a guideline, not a hard rule.

@dhardy
Copy link
Member Author

dhardy commented Jul 10, 2018

BTW @pitdicker if you have the time, would be nice to get your opinion.

@TheIronBorn
Copy link
Collaborator

Sorry, I didn’t mean that we should mention instructions. I should have been clearer.

What I meant was that we should have a policy on very hardware dependent PRNG performance. I don’t know where we stand on that though.

@dhardy
Copy link
Member Author

dhardy commented Jul 10, 2018

Well, should we even have a policy on that? It may be better to leave the performance requirements very vague. If a new PRNG is very fast on some platforms but very slow on others it may still be useful in some cases.

Copy link
Contributor

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

Good job!

Do you plan to add more things to the 'doc' dir? Otherwise can you merge it with CONTRIBUTING.md, or add it as a similar file (PROJECT-POLICIES.MD` etc.)?


## Learning Rand

TODO. In the mean-time, we have some learning resources within the API
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to expand this section someday? I think people rarely want to learn all the details of random number generation, and that the current documentation is ok-ish...

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering whether you had plans for more documentation 😄

Well, for example the current documentation on the PRNG module could be moved. One issue with it now is our plans to relocate these PRNGs to other crates, which doesn't leave a home for that documentation. And it should mention PRNGs from other crates too really.

@@ -16,9 +16,8 @@ The core random number generation traits of Rand live in the [rand_core](
https://crates.io/crates/rand_core) crate; this crate is most useful when
implementing RNGs.

API reference:
[master branch](https://rust-lang-nursery.github.io/rand/rand/index.html),
Copy link
Contributor

Choose a reason for hiding this comment

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

I use this link frequently, and would like to see it stay...

doc/README.md Outdated
- although these are public versions, they are not used by default unless
opting into using a pre-release on the specific `MAJOR.MINOR.PATCH`
version
- pre-releases are considered semantically less than their stem (e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is just me, but I had trouble reading this sentence. Not sure if I have ever seen "stem" used in this context. Would you be ok with something like "less than their release versions"?

doc/README.md Outdated

A trait should define which of its functions are expected to be value-stable.
A type (e.g. a PRNG) is value-stable if all implementations of trait functions
for that type are value-stable where expected to be.
Copy link
Contributor

Choose a reason for hiding this comment

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

You wrote things careful here, maybe with the distinction between from_seed and from_rng in mind? I had to read it a couple of times to completely understand. Maybe:

Both traits and types (such as a PRNG or distribution) should declare whether they are value-stable.
A trait should define for which of its methods it expects the implementations to be value-stable.
When a type that implements such a trait declares it is value-stable, it must keep up these promises.
As an example, SeedableRng::from_seed is required to be value-stable, but SeedableRng::from_rng not. RNGs implementing the trait are value-stable when they guarantee SeedableRng::from_seed is value-stable, while SeedableRng::from_rng may receive optimisations.

@dhardy
Copy link
Member Author

dhardy commented Jul 12, 2018

Updated.

I think we should move CONTRIBUTING.md to this sub-doc too (and maybe rename); it's just neater having everything in one place. (Was there a reason to use such an ugly name? Ah, GitHub.)

@vks
Copy link
Collaborator

vks commented Jul 12, 2018

Looks good to me.

@pitdicker
Copy link
Contributor

This seems ready to merge?

@dhardy
Copy link
Member Author

dhardy commented Jul 23, 2018

Brilliant. I left it open for review but I think this is long enough!

@dhardy dhardy merged commit 686ba14 into rust-random:master Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-docs Documentation D-review Do: needs review E-question Participation: opinions wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants