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

Passing a random number generator to simulator/sampler methods should VS maintaing an internal random state #6567

Open
NoureldinYosri opened this issue Apr 22, 2024 · 1 comment
Labels
kind/design-issue A conversation around design triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add

Comments

@NoureldinYosri
Copy link
Collaborator

NoureldinYosri commented Apr 22, 2024

All Cirq simulators maintain an internal np.random.RandomState

seed: 'cirq.RANDOM_STATE_OR_SEED_LIKE' = None,

This is fine when running in a single thread, however we are starting to have more places where use multiprocessing/multithreads (e.g. using multiprocessing.Pool, concurrent.futures.ThreadPoolExecutor, or other multiprocessing/multithreading libraries) and in these cases this internal random state negatively affects the simulations in two ways

  • the internal random state becomes a shared state that causes the different threads/processes blocked on each other.
  • the results of simulations become correlated.

Suggested solution: Start to prefer passing prngs to methods/functions over maitaining an internal state. this prng should be an np.random.Generator instead of an np.random.RandomState so that we get a spawn method to use when starting threads/processes.

related: #6531

@NoureldinYosri NoureldinYosri added triage/discuss Needs decision / discussion, bring these up during Cirq Cynque kind/design-issue A conversation around design labels Apr 22, 2024
@verult
Copy link
Collaborator

verult commented Apr 24, 2024

From Cirq Cynque: look into a way to convert between np.random.RandomState and np.random.Generator as an alternative approach.

We agreed that adding an RNG argument to each method call (with a default) is useful, but to maintain backward compatibility for the initializer RNG argument, and to make it easier to share the random distribution across method calls, we should still keep an internal RNG as the default fallback if an RNG is not passed into a method call.

@verult verult added triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/design-issue A conversation around design triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add
Projects
None yet
Development

No branches or pull requests

2 participants