Skip to content
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

Return unrecognized random state unmodified #2622

Merged
merged 8 commits into from
Feb 12, 2020

Conversation

kevinsung
Copy link
Collaborator

This allows people to use their own custom pseudorandom number generator implementations.

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Dec 5, 2019
@bryano
Copy link
Collaborator

bryano commented Dec 5, 2019

@kevinsung Do you envision the PRNGs people use being written specifically for Cirq or from a general package? I think a PRNG abstract class would be better than allowing anything, but it would require a Cirq-specific implementation (or at least a wrapper).

@kevinsung
Copy link
Collaborator Author

I have an application where the PRNG is not related to Cirq but can be used as a substitute for a RandomState in appropriate situations.

@bryano
Copy link
Collaborator

bryano commented Dec 5, 2019

Gotchya. In that case, we might as well just set RANDOM_STATE_LIKE to Any. We could get rid of it entirely, but I think it's worth keeping as documentation.

cirq/value/random_state.py Outdated Show resolved Hide resolved
elif isinstance(random_state, int):
return np.random.RandomState(random_state)
raise TypeError(f'Argument must be of type cirq.value.RANDOM_STATE_LIKE.')
else:
return cast(np.random.RandomState, random_state)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a check for one of the expected methods as a safety. Otherwise very pythonic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On further thought, I'd prefer not to make any such check. The user should not be forced to implement any particular method; she should only have to implement the ones that she knows are needed for her use case. For instance, the Cirq simulator only uses prng.choice, while the supremacy circuit generation code only uses prng.random.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per our offline conversation, I have added documentation to RANDOM_STATE_LIKE and left this alone.

@kevinsung kevinsung added the Ready for Re-Review For when reviewers take their time. label Feb 7, 2020
@kevinsung kevinsung dismissed vtomole’s stale review February 12, 2020 19:04

Comment addressed.

@kevinsung kevinsung added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 12, 2020
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 12, 2020
@CirqBot
Copy link
Collaborator

CirqBot commented Feb 12, 2020

Automerge cancelled: Need a fresh 🍪.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 12, 2020
@CirqBot
Copy link
Collaborator

CirqBot commented Feb 12, 2020

Automerge cancelled: The automerge label was removed.

@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 12, 2020
@kevinsung kevinsung merged commit 1b37885 into quantumlib:master Feb 12, 2020
@kevinsung kevinsung deleted the prng branch February 12, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining. Ready for Re-Review For when reviewers take their time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants