Skip to content

Enhancing docs about PRNG seeding - #7068

Merged
sklam merged 7 commits into
numba:masterfrom
jeertmans:random-seeding-warning
Jun 1, 2021
Merged

sklam merged 7 commits into
numba:masterfrom
jeertmans:random-seeding-warning

Conversation

@jeertmans

@jeertmans jeertmans commented May 28, 2021 •

Copy link
Copy Markdown
Contributor

Hello,

This PR directly follows my proposition in #6002.

The goal was to enhance the current documentation to be more clear about PRNG seeding. A note was already present for the Python random module but not the the NumPy one.

It took the liberty to add a minimal working example in both cases, as well as labelling it as a warning to have more impact.

I will gladly accept any comment or suggestion from your side.

Thanks :-)

Updated doc.

Python

python_seed

NumPy

numpy_seed

@esc esc added the 5 - Ready to merge Review and testing done, is ready to merge label May 31, 2021
@esc

esc commented May 31, 2021

Copy link
Copy Markdown
Member

@jeertmans Looks good, thank you!

esc
esc previously approved these changes May 31, 2021

@esc esc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@jeertmans

Copy link
Copy Markdown
Contributor Author

Thanks @esc !

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch. Good to see this documented. A few minor suggestions to resolve else looks good.

Comment thread docs/source/reference/numpysupported.rst Outdated
Comment thread docs/source/reference/numpysupported.rst Outdated
Comment thread docs/source/reference/numpysupported.rst Outdated
Comment thread docs/source/reference/pysupported.rst Outdated
Comment thread docs/source/reference/pysupported.rst Outdated
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 5 - Ready to merge Review and testing done, is ready to merge labels Jun 1, 2021
@stuartarchibald stuartarchibald added this to the Numba 0.54 RC milestone Jun 1, 2021
@stuartarchibald stuartarchibald added the Effort - short Short size effort needed label Jun 1, 2021
jeertmans and others added 5 commits June 1, 2021 15:05
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@jeertmans

Copy link
Copy Markdown
Contributor Author

Thanks for the patch. Good to see this documented. A few minor suggestions to resolve else looks good.

Thanks for the review @stuartarchibald ! Agreed with your suggestions

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch and fixes!

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Jun 1, 2021
@stuartarchibald

Copy link
Copy Markdown
Contributor

Thanks for reviewing @esc.

@sklam
sklam merged commit b29e307 into numba:master Jun 1, 2021
@jeertmans
jeertmans deleted the random-seeding-warning branch June 7, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge Effort - short Short size effort needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants