Added optional prng parameter to sampler and simulator methods - #8289
Added optional prng parameter to sampler and simulator methods#8289AamindMandragora wants to merge 6 commits into
prng parameter to sampler and simulator methods#8289Conversation
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thank you for this work. Here are some initial review comments.
|
|
||
| def get_random_array( | ||
| rng: np.random.RandomState | np.random.Generator, shape: Sequence[int] | None = None | ||
| ): |
There was a problem hiding this comment.
Here and the other new functions in this file lack return type annotations. Could you please add them? (I won't flag every case; they're easy enough to find.)
| rng: np.random.RandomState | np.random.Generator, | ||
| low: int, | ||
| high: int | None = None, | ||
| size: Sequence[int] | None = None, |
There was a problem hiding this comment.
The size parameter is passed to np.random.Generator.integers and np.random.RandomState.randint, but those methods accept both ints and sequences of ints. Can you adjust the type declaration?
| return list(self.run_sweep_iter(program, params, repetitions)) | ||
| if prng is None: | ||
| return list(self.run_sweep_iter(program, params, repetitions)) | ||
| return list(self.run_sweep_iter(program, params, repetitions, prng)) |
There was a problem hiding this comment.
In some cases in the new code, prng is passed as a keyword argument, while in other cases like here, it's passed as a positional argument. It would be better to make them all consistent. Preferably, the keyword argument approach should be used (i.e., typically prng=prng).
| theta = np.arcsin(np.sqrt(random_state.get_random_array(real_rng, shape))) | ||
| phi_d = random_state.get_random_array(real_rng, shape) * np.pi * 2 | ||
| phi_o = random_state.get_random_array(real_rng, shape) * np.pi * 2 |
There was a problem hiding this comment.
I'm not sure what will happen here if shape = (). Could you add a test case for that (if there isn't already one)?
| initial_state: Any, | ||
| qubits: Sequence[cirq.Qid], | ||
| classical_data: cirq.ClassicalDataStore, | ||
| prng: np.random.Generator | None = None, |
There was a problem hiding this comment.
This parameter is not used. This behavior should be documented, or (better) the parameter should be forwarded if the self.state_type method accepts it (e.g., if it's a custom state type that accepts prng).
arettig
left a comment
arettig
left a comment
There was a problem hiding this comment.
Thanks for looking into this! I think a lot of these changes can be removed with some design changes. Ideally, we should leave the RandomState architecture untouched and add separate support for generators, so that we can deprecate RandomState and transition to generators over time.
My suggestions:
- Leave
parse_random_statealone. - Leave samplers alone.
- Change
SimulatorBase._prngto accept either aRandomStateor aGenerator - Change SimulationState to accept either a
RandomStateor aGenerator
There will be a couple other places you have to clean up where RandomState specific methods are used, but probably not that many. Just these couple changes should get us the functionality necessary while being minimally invasive.
3 participants
Should complete #6567
Threaded an optional
prng: np.random.Generator | Nonethrough sampler and simulator APIs, defaults toNone, allrun_batchandrun_batch_asynccallprng.spawn(len(programs))to give each circuit its own child generator, making batched runs independent, added helpers that are agnostic toGeneratorvsRandomState, still backwards-compatible with simulators and samplers that don't haveprng.