Repository navigation
Conversation
Replaces the use of RandomState in the random circuit generators with numpy Generators. Replaces cases where the tests depended on the random seeding/algorithm.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8527 +/- ##
==========================================
- Coverage 99.58% 99.58% -0.01%
==========================================
Files 1137 1137
Lines 105119 105133 +14
==========================================
+ Hits 104687 104698 +11
- Misses 432 435 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pavoljuhas
left a comment
There was a problem hiding this comment.
LGTM after addressing some small issues (see inline) and a few leftover references to np.random.RandomState in touched files.
Thank you for this conversion!
| qubits = cirq.LineQubit.range(num_qubits) | ||
| circuit = cirq.Circuit() | ||
| random_state = cirq.value.parse_random_state(random_state) | ||
| random_state = cirq.value.parse_random_generator(random_state) |
There was a problem hiding this comment.
Nit - should we rename LHS to prng here?
| @@ -33,7 +33,7 @@ def get_seeded_initial_mapping(graph_seed, init_seed) -> dict[cirq.Qid, cirq.Qid | |||
|
|
|||
| @pytest.mark.parametrize('seed', [random.randint(0, 2**32) for _ in range(10)]) | |||
There was a problem hiding this comment.
Nit - can we replace the above with one static sample, i.e., a fixed list of 10 integers generated as above?
This would let us have constant parameterized test names (which include parameter value) with effectively the same test.
|
|
||
|
|
||
| def test_benchmark_2q_xeb_fidelities_vectorized() -> None: | ||
| rs = np.random.RandomState(52) |
There was a problem hiding this comment.
I think we can replace these instances of RandomState - the patch below seems to work:
diff --git a/cirq-core/cirq/experiments/xeb_fitting_test.py b/cirq-core/cirq/experiments/xeb_fitting_test.py
index bd1fb45c9..363e0681c 100644
--- a/cirq-core/cirq/experiments/xeb_fitting_test.py
+++ b/cirq-core/cirq/experiments/xeb_fitting_test.py
@@ -142,6 +142,8 @@ def test_benchmark_2q_xeb_fidelities_parallel() -> None:
def test_benchmark_2q_xeb_fidelities_vectorized() -> None:
- rs = np.random.RandomState(52)
- mock_records = [{'pure_probs': rs.rand(4), 'sampled_probs': rs.rand(4)} for _ in range(100)]
+ prng = cirq.value.parse_random_generator(52)
+ mock_records = [
+ {'pure_probs': prng.random(4), 'sampled_probs': prng.random(4)} for _ in range(100)
+ ]
df = pd.DataFrame(mock_records)
@@ -307,7 +309,7 @@ def test_parallel_full_workflow(request, use_pool) -> None:
def test_fit_exponential_decays() -> None:
- rs = np.random.RandomState(999)
+ prng = cirq.value.parse_random_generator(999)
cycle_depths = np.arange(3, 100, 11)
- fidelities = 0.95 * 0.98**cycle_depths + rs.normal(0, 0.2)
+ fidelities = 0.95 * 0.98**cycle_depths + prng.normal(0, 0.02)
a, layer_fid, a_std, layer_fid_std = _fit_exponential_decay(cycle_depths, fidelities)
np.testing.assert_allclose([a, layer_fid], [0.95, 0.98], atol=0.02)
@@ -317,7 +319,7 @@ def test_fit_exponential_decays() -> None:
def test_fit_exponential_decays_negative_fids() -> None:
- rs = np.random.RandomState(999)
+ prng = cirq.value.parse_random_generator(999)
cycle_depths = np.arange(3, 100, 11)
- fidelities = 0.5 * 0.5**cycle_depths + rs.normal(0, 0.2) - 0.5
+ fidelities = 0.5 * 0.5**cycle_depths + prng.normal(0, 0.2) - 0.5
assert np.sum(fidelities > 0) <= 1, 'they go negative'
a, layer_fid, a_std, layer_fid_std = _fit_exponential_decay(cycle_depths, fidelities)
| random_state: A seed (int) or `np.random.Generator` to use when | ||
| generating random values. If not set, a new unseeded generator is | ||
| used. |
There was a problem hiding this comment.
Nit on wording - feel free to adjust:
| random_state: A seed (int) or `np.random.Generator` to use when | |
| generating random values. If not set, a new unseeded generator is | |
| used. | |
| random_state: A seed (int) or `np.random.Generator` to use when | |
| generating random values. Use a default unseeded generator | |
| when unspecified. |
Also please check and sync docstrings of other functions here that take random_state.
| rng: Random number generator (or seed) to be used in sampling. Anything | ||
| accepted by `cirq.value.parse_random_generator`. Default is a new, | ||
| unseeded `np.random.Generator`. |
There was a problem hiding this comment.
Nit:
| rng: Random number generator (or seed) to be used in sampling. Anything | |
| accepted by `cirq.value.parse_random_generator`. Default is a new, | |
| unseeded `np.random.Generator`. | |
| rng: Random number generator (or seed) to be used in sampling. Anything | |
| accepted by `cirq.value.parse_random_generator`. If unspecified, | |
| use a default unseeded `np.random.Generator`. |
| """ | ||
| real_rng = random_state.parse_random_state(rng) | ||
| real_rng = random_state.parse_random_generator(rng) | ||
| size = tuple(shape) |
There was a problem hiding this comment.
Nit - size can be confusing as it evokes a scalar. Let us just coerce shape or, if type checker complains, use a new variable tshape.
| size = tuple(shape) | |
| shape = tuple(shape) |
| tab = two_qubit_gate_product_tabulation( | ||
| base_gate, 0.1, sample_scaling=2, random_state=np.random.RandomState(seed) | ||
| ) |
There was a problem hiding this comment.
| tab = two_qubit_gate_product_tabulation(base_gate, 0.1, sample_scaling=2, random_state=seed) | |
| ) |
|
|
||
|
|
||
| def _cz_with_adjacent_z_rotations( | ||
| a: cirq.GridQubit, b: cirq.GridQubit, prng: np.random.RandomState |
There was a problem hiding this comment.
| a: cirq.GridQubit, b: cirq.GridQubit, prng: np.random.Generator |
|
|
||
|
|
||
| def random_locals(x: float, y: float, z: float, seed=None) -> np.ndarray: | ||
| rng = np.random.RandomState(seed) |
There was a problem hiding this comment.
rng = cirq.value.parse_random_generator(seed)
|
|
||
|
|
||
| def test_numerical_compiler_json_rejects_live_rng() -> None: | ||
| compiler = TwoQubitNumericalCompiler(base_gates=(_CZ,), random_state=np.random.RandomState(1)) |
There was a problem hiding this comment.
| compiler = TwoQubitNumericalCompiler(base_gates=(_CZ,), random_state=np.random.default_rng()) |
Part of #8374
This replaces the usage of
parse_random_statewith the newparse_random_generatorin cirq-core. Several tests required specific seeds to pass and these were fixed in several ways:Potentially breaking behavior: