-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Add randomized clifford+T benchmarking circuits. #2118
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.
Hello @FarLab, thank you for submitting a PR to Mitiq! We will respond as soon as possible, and if you have any questions in the meantime, you can ask us on the Unitary Fund Discord.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2118 +/- ##
==========================================
- Coverage 98.31% 98.19% -0.12%
==========================================
Files 87 88 +1
Lines 4142 4166 +24
==========================================
+ Hits 4072 4091 +19
- Misses 70 75 +5 ☔ View full report in Codecov by Sentry. |
The function Moreover, even if it was originally suggested in the issue, now I wonder if we should simply return the circuit without the associated heavy bitstrings. Heavy bitstrings are important for quantum volume circuits and I am not sure if the are important in this case. Moreover the user can always call the function So, I would suggest to simply don't consider heavy bitstrings in this new benchmark. But please wait for a second opinion by @natestemen about this. I don't want to make you remove some code that other people think it is instead useful. Further unrelated comment: please add the new function in the API docs in this file |
co-authored-by: Farrokh Labib <farrokh@unitary.fund> co-authored-by: Misty Wahl <misty@unitary.fund>
co-authored-by: Farrokh Labib <farrokh@unitary.fund> co-authored-by: Misty Wahl <misty@unitary.fund>
Thanks Andrea for your comments. I will probably remove the heavy bitstring function since it already exists. I will also add the function in the API docs. |
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.
Really nice work here Farrokh! A great first addition to Mitiq.
I've left a few small comments, but my biggest is a small concern that in order to appease mypy, we've made the code quite a bit more complicated by shifting everything to manipulating indices. I feel like a simpler approach would be to cast all the objects that get passed to np.random.choice
as np.ndarray
s. WDYT?
|
||
Returns: | ||
A quantum circuit acting on ``num_qubits`` qubits. | ||
A list of the heavy bitstrings for the returned circuit. |
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 think this line should be removed now that we don't return bitstrings.
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 catch, will remove!
oneq_idx_list = [ | ||
rnd_state.choice(list(range(len(oneq_cliffords)))) for _ in range(3) | ||
] | ||
twoq_idx_list = [ | ||
rnd_state.choice(list(range(len(twoq_cliffords)))) for _ in range(3) | ||
] |
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 think the range
functions here should be range(num_oneq_cliffords)
and range(num_twoq_cliffords)
respectively. I think the 3 might be a magic number.
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.
yes, you are right, that should not be 3 (which was probably hardcoded for some reason)
oneq_idx_list = [ | ||
rnd_state.choice(list(range(len(oneq_cliffords)))) for _ in range(3) | ||
] | ||
twoq_idx_list = [ | ||
rnd_state.choice(list(range(len(twoq_cliffords)))) for _ in range(3) | ||
] |
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.
If we have to keep all the indices, this is a little cleaner.
oneq_idx_list = [ | |
rnd_state.choice(list(range(len(oneq_cliffords)))) for _ in range(3) | |
] | |
twoq_idx_list = [ | |
rnd_state.choice(list(range(len(twoq_cliffords)))) for _ in range(3) | |
] | |
oneq_idx_list = rnd_state.choice(2, num_oneq_cliffords) | |
twoq_idx_list = rnd_state.choice(2, num_twoq_cliffords) |
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 like this solution more that casting the objects that go into np.random.choice as np.ndarray. also, I think the 2 should be len(oneq_cliffords) and len(twoq_cliffords), right?
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.
That's right!
) | ||
|
||
|
||
def test_generate_model_circuit_with_seed(): |
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 think this test name is copied from another file, and I know we use it in other places, but I think we can make it a little more descriptive of what the actual intention is. Maybe something like test_seed_circuit_equality
? I don't love that name either, but I'm sure we can come up something better than "model circuit".
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.
test_seed_circuit_equality sounds good to me. we can change it if we find a better name :)
"""Tests for quantum volume circuits. The Cirq functions that do the main work | ||
are tested here: | ||
cirq-core/cirq/contrib/quantum_volume/quantum_volume_test.py | ||
|
||
Tests below check that generate_quantum_volume_circuit() works as a wrapper and | ||
fits with Mitiq's interface. | ||
""" |
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.
comment needs removal, or updating.
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
num_twoq_cliffords=2, | ||
num_t_gates=2, | ||
return_type=return_type, | ||
seed=3, |
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.
Let's remove this line just so it's running the test with random circuit each time.
seed=3, |
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.
agree
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.
One small comment needs updating, and we'll need to add a line into mitiq/benchmarks/__init__.py
to make these functions importable like
from mitiq.benchmarks import generate_random_clifford_t_circuit
Once those two things are done, we are good to go!
# This source code is licensed under the GPL license (v3) found in the | ||
# LICENSE file in the root directory of this source tree. | ||
|
||
"""Functions for generating rotated randomized benchmarking circuits.""" |
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.
Needs update.
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 catch, I will update these two last points now
…in mitiq/benchmarks/__init__.py to make the functions importable
mitiq/benchmarks/__init__.py
Outdated
@@ -14,3 +14,4 @@ | |||
) | |||
from mitiq.benchmarks.w_state_circuits import generate_w_circuit | |||
from mitiq.benchmarks.qpe_circuits import generate_qpe_circuit | |||
from mitiq.benchmarks import generate_random_clifford_t_circuit |
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.
We need to add the full path here so that when importing this from mitiq.benchmarks
is possible. I think it's going to look something like this (there might be a formatting change, I'm not 100% sure).
from mitiq.benchmarks import generate_random_clifford_t_circuit | |
from mitiq.benchmarks.randomized_clifford_t_circuit import ( | |
generate_random_clifford_t_circuit, | |
) |
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.
thanks, als checked the formatting and it didn't complain :)
docs/source/apidoc.md
Outdated
@@ -241,8 +267,15 @@ See Ref. {cite}`Czarnik_2021_Quantum` for more details on these methods. | |||
:members: | |||
``` | |||
|
|||
#### Randomized Clifford+T benchmarking Circuits |
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.
#### Randomized Clifford+T benchmarking Circuits | |
#### Randomized Clifford+T Circuits |
…rrectly and a small change in the apidoc.md
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 work Farrokh! Can't wait to do some more testing with these circuits!
🚢
Description
This PR adds the feature requested here #1230.
closes #1230
License
Before opening the PR, please ensure you have completed the following where appropriate.