-
Notifications
You must be signed in to change notification settings - Fork 559
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
Update rand() documentation to be more modern and clear #22727
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bram-perl
reviewed
Nov 8, 2024
bram-perl
reviewed
Nov 8, 2024
Any more advice on fixes/updates on wording so this can get merged? |
On Tue, 12 Nov 2024, 14:20 guest20, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pod/perlfunc.pod
<#22727 (comment)>:
> -(Note: If your rand function consistently returns numbers that are too
+=item *
+
+Currently EXPR with the value C<0> is
+also special-cased as C<1>. This was undocumented before Perl 5.8.0
+and is subject to change in future versions of Perl.
+
+=item *
+
+As of Perl v5.20.0 C<rand> uses the C<drand48()> PRNG to generate random numbers.
+This should be acceptable for educational or hobby projects, basic simulations,
+or simple random sampling.
+
+=item *
+
+If your C<rand> function consistently returns numbers that are too
your C<rand>
This refers to the rand in the libs/OS you picked. This type of language
is used to couch all sorts of porting issues.
Any such language should be removed, we added the hand rolled drand48()
specifically to insulate ourselves from OS dependencies. I don't remember
if we support a build option to use the system rand/srand (but probably we
do), if we do then we can keep these comments but caveat them
appropriately; a normal build will use the same (bad) rng everywhere.
Yves
… |
On Tue, 12 Nov 2024, 06:44 Scott Baker, ***@***.***> wrote:
Any more advice on fixes/updates on wording so this can get merged?
FWIW i don't think we should say that our rand is suitable for any specific
purpose. I think we should say that it is not suitable for any
mathematically rigorous purpose and leave it to the reader to make their
own decisions. Perhaps suggesting some alternatives like MT or WELL
variants. WELL-512 is probably one of the better choices these days.
Linear congruent random number generators as a whole have a variety of
issues, and the one we provide was modern when 16 bit integers were modern
and has less state (48 bits of course) than an integer in a standard
build. To make things worse iirc we don't even dispose of the high bits in
the state, which are substantially less random than the low bits.
These days the state of art of non cryptographic generators with limited
state is much advanced and people really shouldn't use rand() for anything
serious at all. I really wish we bundled a better RNG.
Yves
… |
scottchiefbaker
force-pushed
the
blead
branch
from
November 13, 2024 16:27
0abdb22
to
e9749bf
Compare
@demerphq I updated the two sections you referenced. |
On Thu, 14 Nov 2024, 03:32 Scott Baker, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pod/perlfunc.pod
<#22727 (comment)>:
> +As of Perl v5.20.0 C<rand> uses the C<drand48()> PRNG to generate random numbers.
+This should be acceptable for educational or hobby projects, basic simulations,
+or simple random sampling.
+
I took out the wording that ChatGPT used
Thanks.
Unfortinately it still says: "As a PRNG drand48() should be sufficient for
most non-cryptographic needs."
I don't think this is correct, an LCG PRNG is not suitable for many tasks,
for instance if you use an LCG to choose coordinates you will find that the
coordinates chosen all lie on a predictable hyper-lattice. (Wikipedia has a
nice animated gif showing this IIRC)
IMO Drand48 is basically only suitable for simple sampling tasks which do
not require any mathematical rigor.
Cheers
Yves
|
leonerd
reviewed
Nov 20, 2024
leonerd
reviewed
Nov 21, 2024
leonerd
approved these changes
Nov 21, 2024
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.
Needs one letter case adjustment but aside from that, LGTM
@leonerd good catch... docs updated |
@scottchiefbaker These commits should be squashed before we merge |
scottchiefbaker
force-pushed
the
blead
branch
2 times, most recently
from
November 21, 2024 18:27
1548c66
to
0c4f56b
Compare
scottchiefbaker
force-pushed
the
blead
branch
from
November 21, 2024 18:29
0c4f56b
to
41e43bc
Compare
scottchiefbaker
force-pushed
the
blead
branch
from
November 21, 2024 22:35
d9ac81b
to
84f174a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Learning more about how Perl implements random numbers lead me to clean up and update the documentation.