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 support for np.random.Generator #6566
base: main
Are you sure you want to change the base?
Add support for np.random.Generator #6566
Changes from 4 commits
834ee27
9ce26d6
30a73e7
f455207
c6cfb37
0fc07df
d5ade4f
1e9c7ab
aff9556
b06f456
770e8fe
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.
The return type of different types tends to be a code smell. Such returned value is less useful for type checking. In addition, Generator and RandomState (not to mention _CUSTOM_PRNG_T) have different APIs so the
parse_prng
caller would still need to do someisinstance
check to ascertain the actual type and figure what methods can be called.I would propose an alternative approach:
(1) convert the
RANDOM_STATE_OR_SEED_LIKE
type from Any to the Union of numpy types that can be converted to RandomState and the np.random.Generator type. Hopefully this can be done without too much hassle with typechecks, because the current Any type skips them completely.(2) extend parse_random_state to accept a Generator object and convert it to RandomState.
Generator-s have
bit_generator
attribute that can be used to create RandomState.(3) add method
parse_random_generator
to thecirq.value.random_state
module which would takeRANDOM_STATE_OR_SEED_LIKE
argument and convert it to a Generator object.np.random.RandomState()
has a_bit_generator
attribute that can be used for creating a Generator.If in some configurations the
_bit_generator
is not present, we can just use RandomState.randint to get a seed for thenp.random.default_rng()
With these steps in place, we can keep all the existing interfaces that take
RANDOM_STATE_OR_SEED_LIKE
and just start replacing its interpretation fromparse_random_state
toparse_random_generator
as needed.This would also avoid bifurcation between RANDOM_STATE_OR_SEED_LIKE and PRNG_OR_SEED_LIKE types that may need several major releases to clear up.
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.
ptal
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 don't think we need this. One output from
random()
is enough to check if 2 generators are at the same seed.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.
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.
Let us not check cross-group inequality. Following the
test_parse_random_state
style is a bit more readableThere 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.
This is a noop check for a single value. Perhaps replace with
if you are OK with the previous suggestion to have a singleton generator for None.