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.
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
Add SPEC7: Seeding pseudo-random number generation #180
Add SPEC7: Seeding pseudo-random number generation #180
Changes from 11 commits
b233242
1e0d3f9
009453f
cfa4fd8
9bf299a
fb64c31
d55c0b2
935fed2
a17eba9
0703ee8
3fd8fc7
7c163df
e2d6541
9894af0
8d89cc8
151d68d
5491549
f4cf078
9d60785
7531111
c196b74
415f173
efc6428
83a1d92
7a4258e
4679d40
14ab4a4
07a2235
07744f2
be41cd6
4b1c1c4
a6e1caa
38341b1
63a7bf5
ef35bb9
4f8a441
e6cf422
a7d844f
83b28b6
cef9c51
f894a7a
044ff76
27934c7
636bc5d
bd32f7b
6ec3b6a
4aa7051
37ff883
2dcf128
cc43293
7b4c61b
ab3f448
0ae7919
b7ddc1f
81da885
b106497
1180190
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
For simplicity, can this reference to
BitGenerator
be removed? For this SPEC, the user facing API is aroundGenerator
andRandomState
.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.
Suggestion taken; I am waiting to see how the rest of the text pans out, to see whether we ever need to explain the notion of a bitgenerator. I quite like explaining it this way, since those are fundamental building blocks, but also fine to leave it out if it does not add anything.
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 recommend splitting this off into a separate
## Motivation
section which goes above the Implementation section. That will help the reader, and it will make clearer to the authors how short the motivation given now is (it needs expanding).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 do.
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.
Partially addressed by stefanv@2dcf128, but I leave addition of motivation for a future PR. I'm not the best person to comment on this; I was comfortable controlling using
np.random.rand
and only changed due to peer pressure : )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.
That links back to my comment - I think you wanted to link a commit?
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.
Yup, fixed it - stefanv@2dcf128. But I just moved the section; didn't add additional motivation.
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.
The SPEC template doesn't have a motivation section (/cc @jarrodmillman perhaps we should consider), but I added some motivational text to the description.
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.
Note that this decorator uses the deprecated
random_state
while point 2 above talks aboutseed
as the deprecated parameter? Is this a typo or am I confused myself?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.
Ah, should align it to
random_state
probably. SciPy has both, which is where this slip up cames from probably.