Added sweeps for number of qubits #47
Conversation
|
|
| SECRET_STRING = bin(random.getrandbits(i - 1))[2:] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("num_qubits", sweep_list) |
There was a problem hiding this comment.
I don't think using the decorator here to parameterize this function will be sufficient to update the benchmark to run with n qubits. I think you'll need to add the decorator to the bench_qiskit_bv function and take num_qubits as a parameter on the function definition. Then from there you'll need to update the function's code to call build_bv_circuit with the secret string from the parameter and pass that returned circuit directly to run_qiskit_circuit. Right now the benchmark function is hard coded to load the circuit from the in repo qasm file.
There was a problem hiding this comment.
I tried doing this, is the way I implemented it correct?
There was a problem hiding this comment.
This is moving in the right direction, I left a few inline comments on some potential issues and places for cleanup that I caught just from reading the code. As we discussed offline the biggest blocker I think is the requests import which will cause an ImportError, but besides that I'm not sure if the fixture usage is correct to generate the parameter list for num_qubits. We'll have to refer to https://docs.pytest.org/en/6.2.x/fixture.html for more details on the api around this.
| if __name__ == "__main__": | ||
| build_bv_circuit(SECRET_STRING).qasm(filename=os.path.join(QASM_DIR, "bv.qasm")) | ||
| build_bv_circuit(SECRET_STRING, True).qasm(filename=os.path.join(QASM_DIR, "bv_mcm.qasm")) | ||
| build_bv_circuit(default_secret_string).qasm(filename=os.path.join(QASM_DIR, "bv.qasm")) | ||
| build_bv_circuit(default_secret_string, True).qasm(filename=os.path.join(QASM_DIR, "bv_mcm.qasm")) |
There was a problem hiding this comment.
Since we're changing the execution model of these benchmarks to no longer depend on hard coded qasm files we can delete this block of code now as we don't need to regenerate qasm files. Similarly we can remove the bv*.qasm files at the same time because nothing will use them anymore.
| @@ -13,9 +12,30 @@ | |||
|
|
|||
| from red_queen.games.applications import backends, run_qiskit_circuit | |||
|
|
|||
| import random | |||
|
|
|||
| import requests | |||
There was a problem hiding this comment.
As we discussed offline, I don't think this import is needed and is likely the cause of the errors on import since requests is an external python library for HTTP: https://pypi.org/project/requests/ and it's not being part of red queens requirements.
|
|
||
|
|
||
| sweep_list = list(requests.get(get_num_qubits)) | ||
| default_secret_string = "110011" |
There was a problem hiding this comment.
I expect this will fail pylint because it likes constants to be all caps, but you can check with tox -elint to confirm and fix it if it does have issues.
| else: | ||
| benchmark.name = "Bernstein Vazirani (mid-circuit measurement)" | ||
| circ = QuantumCircuit.from_qasm_file(os.path.join(QASM_DIR, "bv_mcm.qasm")) | ||
| circ = build_bv_circuit(SECRET_STRING, True) |
There was a problem hiding this comment.
We should check that the number of circuit qubits is <= the number of backend qubits. If not this will cause a runtime error and it'd be better to handle that gracefully.
| shots = 65536 | ||
| SECRET_STRING = str(bin(random.getrandbits(num_qubits - 1))[2:].zfill(num_qubits-1)) |
There was a problem hiding this comment.
The use of random here is potentially problematic from a reproducibility perspective. Since we're expecting to run these benchmarks for comparison purposes the selected secret string will potentially be different between runs for a fixed input parameter set. Using an random number generator to pick the exact secret string is fine but we should set it with a fixed seed: https://docs.python.org/3/library/random.html#random.seed to ensure the values are consistent/reproducible between runs.
| sweep = np.linspace(int(user_params[1]), int(user_params[2]), int(user_params[3])) | ||
| sweep = [int(i) for i in sweep] |
There was a problem hiding this comment.
You can simplify this by specifying the dtype directly:
| sweep = np.linspace(int(user_params[1]), int(user_params[2]), int(user_params[3])) | |
| sweep = [int(i) for i in sweep] | |
| sweep = np.linspace(int(user_params[1]), int(user_params[2]), int(user_params[3]), dtype=int) |
This will return a numpy array with an integer datatype
| return sweep | ||
|
|
||
|
|
||
| sweep_list = list(requests.get(get_num_qubits)) |
There was a problem hiding this comment.
I think this might need to be updated to call the function directly without requests.
Added parameter
--num_qubitswhich allows users to specify either a linear or logarithmic sweep for the number of qubits inrun_ft.pyandrun_bv.pybased on user specifications.