Use local random seeds in slater determinant tests#1308
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates the JWGetGaussianStateTest class to use reproducible random number generation by introducing numpy.random.default_rng and SeedSequence. Feedback from the reviewer indicates that calling spawn(1) inside loops causes the same seed to be generated for every iteration, which reduces test diversity. Additionally, the reviewer pointed out that maintaining both a Generator and a SeedSequence with the same initial seed is redundant and could lead to correlated random streams, suggesting instead that a single generator be used for all random choices.
| def setUp(self): | ||
| self.n_qubits_range = range(2, 10) | ||
| self.rng = numpy.random.default_rng(5387) | ||
| self.hamiltonian_seed_sequence = numpy.random.SeedSequence(5387) |
There was a problem hiding this comment.
The SeedSequence is redundant here because self.rng (a Generator) can be used to produce seeds for the Hamiltonian construction. Additionally, using the same seed (5387) for both default_rng and SeedSequence can lead to correlated random streams. It is recommended to use a single generator for all random choices in the test to ensure independence and simplify the code.
| self.hamiltonian_seed_sequence = numpy.random.SeedSequence(5387) | |
| # self.hamiltonian_seed_sequence is not needed if we use self.rng |
| hamiltonian_seed = self.hamiltonian_seed_sequence.spawn(1)[0].generate_state(1)[0] | ||
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, True, seed=hamiltonian_seed) |
There was a problem hiding this comment.
Calling spawn(1) inside the loop returns the same child SeedSequence in every iteration, which results in the same hamiltonian_seed being used for all values of n_qubits. This reduces the diversity of the test cases. Use the existing self.rng to generate a unique seed for each iteration.
| hamiltonian_seed = self.hamiltonian_seed_sequence.spawn(1)[0].generate_state(1)[0] | |
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, True, seed=hamiltonian_seed) | |
| hamiltonian_seed = int(self.rng.integers(0, 2**32)) | |
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, True, seed=hamiltonian_seed) |
| hamiltonian_seed = self.hamiltonian_seed_sequence.spawn(1)[0].generate_state(1)[0] | ||
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, False, seed=hamiltonian_seed) |
There was a problem hiding this comment.
Calling spawn(1) inside the loop returns the same child SeedSequence in every iteration, which results in the same hamiltonian_seed being used for all values of n_qubits. Use the existing self.rng to generate a unique seed for each iteration.
| hamiltonian_seed = self.hamiltonian_seed_sequence.spawn(1)[0].generate_state(1)[0] | |
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, False, seed=hamiltonian_seed) | |
| hamiltonian_seed = int(self.rng.integers(0, 2**32)) | |
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, False, seed=hamiltonian_seed) |
| hamiltonian_seed = self.hamiltonian_seed_sequence.spawn(1)[0].generate_state(1)[0] | ||
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, True, seed=hamiltonian_seed) |
There was a problem hiding this comment.
Calling spawn(1) inside the loop returns the same child SeedSequence in every iteration, which results in the same hamiltonian_seed being used for all values of n_qubits. Use the existing self.rng to generate a unique seed for each iteration.
| hamiltonian_seed = self.hamiltonian_seed_sequence.spawn(1)[0].generate_state(1)[0] | |
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, True, seed=hamiltonian_seed) | |
| hamiltonian_seed = int(self.rng.integers(0, 2**32)) | |
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, True, seed=hamiltonian_seed) |
| hamiltonian_seed = self.hamiltonian_seed_sequence.spawn(1)[0].generate_state(1)[0] | ||
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, False, seed=hamiltonian_seed) |
There was a problem hiding this comment.
Calling spawn(1) inside the loop returns the same child SeedSequence in every iteration, which results in the same hamiltonian_seed being used for all values of n_qubits. Use the existing self.rng to generate a unique seed for each iteration.
| hamiltonian_seed = self.hamiltonian_seed_sequence.spawn(1)[0].generate_state(1)[0] | |
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, False, seed=hamiltonian_seed) | |
| hamiltonian_seed = int(self.rng.integers(0, 2**32)) | |
| quadratic_hamiltonian = random_quadratic_hamiltonian(n_qubits, False, seed=hamiltonian_seed) |
This updates the Slater determinant Gaussian-state tests to keep their random choices local to the test case. The Hamiltonian construction now receives explicit per-test seeds, and the excited-state orbital selections use a local NumPy generator instead of the module-level random state, which makes this module less dependent on the global pytest seed while preserving deterministic coverage. Verified with
python3 -m compileall -q src/openfermion/circuits/slater_determinants_test.py,python3 -m pytest src/openfermion/circuits/slater_determinants_test.py -q -k 'JWGetGaussianStateTest', andgit diff --check. Part of #1060.