-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
Doc and policies #544
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocks are checked
?
Perhaps we should mention PRNG portability/hardware support? |
@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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
BTW @pitdicker if you have the time, would be nice to get your opinion. |
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. |
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. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
Updated. I think we should move |
Looks good to me. |
This seems ready to merge? |
Brilliant. I left it open for review but I think this is long enough! |
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?