-
Notifications
You must be signed in to change notification settings - Fork 984
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
Speed up some over parameterized tests #5606
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nit
@@ -58,9 +58,12 @@ def _all_rotation_pairs(): | |||
yield (px, flip_x), (pz, flip_z) | |||
|
|||
|
|||
@functools.cache | |||
def _all_clifford_gates(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line not touched but might as well add a return type annotation while we're at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it made me realize this likely doesn't work since List is not hashable....so changed to tuple!
Looks like |
Naive python question: Why do these functions have different return types? from typing import Generator, Tuple
def tuple_of_ints() -> Tuple[int, ...]:
return tuple(
i for i in range(2)
)
def generator_of_tuple_of_ints() -> Generator[int, None, None]:
return (
i for i in range(2)
) |
You might think |
@@ -739,7 +740,7 @@ def test_multi_clifford_decompose_by_unitary(): | |||
# Construct a random clifford gate: | |||
n, num_ops = 5, 20 # because we relied on unitary cannot test large-scale qubits | |||
gate_candidate = [cirq.X, cirq.Y, cirq.Z, cirq.H, cirq.S, cirq.CNOT, cirq.CZ] | |||
for seed in range(100): | |||
for seed in range(10): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to drop seeding here and replace it with np.random calls and a simple repetition.
The default numpy random generator is getting reproducibly, non-constantly seeded
in check/pytest. Over time there should be more randomness in its generated choices than with the 10 seeds here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
LGTM, but I have noticed some strange random seeding in the test with seeds from Perhaps a matter for separate PR. |
No description provided.