Skip to content
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

[Classical Shadows] quantum processing and test #1906

Merged
merged 84 commits into from
Jul 21, 2023

Conversation

Min-Li
Copy link
Contributor

@Min-Li Min-Li commented Jul 9, 2023

Description

z basis measurement and test from #1899


License

  • I license this contribution under the terms of the GNU GPL, version 3 and grant Unitary Fund the right to provide additional permissions as described in section 7 of the GNU GPL, version 3.

Before opening the PR, please ensure you have completed the following where appropriate.

@github-actions
Copy link

github-actions bot commented Jul 9, 2023

Binder 👈 Launch a binder notebook on branch Min-Li/mitiq/shadows/z-basis-measurement

@Min-Li Min-Li changed the title [Classical Shadows] Z-basis measurement and test [Classical Shadows 2] Z-basis measurement and test Jul 10, 2023
Copy link
Member

@natestemen natestemen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Min-Li it looks like a few of my previous comments were resolved without a change / comment. I unresolved a few of my previous comments. Please make sure to leave a comment if you are not going to make the requested change before resolving.

mitiq/shadows/quantum_processing.py Outdated Show resolved Hide resolved
Min-Li and others added 2 commits July 16, 2023 23:19
Co-authored-by: nate stemen <nate@stemen.email>
Co-authored-by: nate stemen <nate@stemen.email>
@Min-Li
Copy link
Contributor Author

Min-Li commented Jul 17, 2023

@natestemen Hi Nate, I resolved some conversations without leaving comments since I already address your comments in some previous commits. I will add comments before resolving conversations later.

Copy link
Member

@nathanshammah nathanshammah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @Min-Li, LGTM, for the api-doc part.

Copy link
Member

@andreamari andreamari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Min-Li, good form my side. But please let's wait for Nate's approval before merging this PR.

I also left a suggestion to explain that the executor is expected to return a single shot.

mitiq/shadows/quantum_processing.py Outdated Show resolved Hide resolved
@nathanshammah nathanshammah removed the request for review from Misty-W July 17, 2023 15:18
@Min-Li Min-Li requested a review from natestemen July 17, 2023 15:42
@Min-Li Min-Li mentioned this pull request Jul 18, 2023
6 tasks
Copy link
Member

@natestemen natestemen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work Min. I think this is pretty much ready to go from my POV.

Please make sure to add tqdm to our dev_requirements.txt file.

mitiq/shadows/quantum_processing.py Show resolved Hide resolved
mitiq/shadows/quantum_processing.py Outdated Show resolved Hide resolved
mitiq/shadows/quantum_processing.py Show resolved Hide resolved
Comment on lines 207 to 230
@pytest.mark.parametrize("executor", [cirq_executor, qiskit_executor])
def test_random_pauli_measurement_time_power_growth(
executor: Callable,
):
"""Test that random_pauli_measurement scales follow power law with the
number of measurements."""
n_qubits = [3, 6, 9, 12, 15]

times = []
for n in n_qubits:
qubits = cirq.LineQubit.range(n)
circuit = simple_test_circuit(qubits)
start_time = time.time()
random_pauli_measurement(
circuit,
100, # number of total measurements
executor=executor,
)

times.append(time.time() - start_time)
for i in range(1, len(times)):
log_ratio_times = np.log(times[i] / times[i - 1])
log_ratio_qubits = np.log(n_qubits[i] / n_qubits[i - 1])
assert log_ratio_times == pytest.approx(log_ratio_qubits, rel=5)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we remove this test? It's quite slow, and it's not testing the correctness of the code. I ran pytest mitiq/shadows --durations=15 to get the tests ordered by speed in descending order.

==================================================================== slowest 15 durations =============================================
13.96s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_time_power_growth[qiskit_executor]
3.96s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_time_growth[qiskit_executor]
1.14s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_time_power_growth[cirq_executor]
0.40s call     mitiq/shadows/test/test_quantum_processing.py::test_generate_random_pauli_strings_time
0.30s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_output_types[qiskit_executor-2]
0.30s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_output_dimensions[qiskit_executor-5]
0.30s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_output_dimensions[qiskit_executor-2]
0.29s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_no_errors[qiskit_executor-5]
0.29s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_output_types[qiskit_executor-5]
0.28s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_no_errors[qiskit_executor-1]
0.27s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_no_errors[qiskit_executor-2]
0.20s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_output_types[qiskit_executor-1]
0.20s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_output_dimensions[qiskit_executor-1]
0.15s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_time_growth[cirq_executor]
0.01s call     mitiq/shadows/test/test_quantum_processing.py::test_random_pauli_measurement_output_dimensions[cirq_executor-5]

@Misty-W
Copy link
Contributor

Misty-W commented Jul 20, 2023

Nice work Min. I think this is pretty much ready to go from my POV.

Please make sure to add tqdm to our dev_requirements.txt file.

@natestemen, @Min-Li, I hesitate to add a new dependency to Mitiq. My understanding is that tqdm is a status bar, please correct me if I'm missing something. Is there a particular reason why a status bar is a "must have" for Classical Shadows?

@nathanshammah
Copy link
Member

Wanted just to comment that the errors in CI in build-docs occurred in the bqskit tutorial. As they are unrelated to these changes, they can be disregarded.

…` if len(result.get_counts().keys()) > 1:

            assert len(result.get_counts().keys()) == 1, (
                "The `executor` must return a `MeasurementResult`"
                " for a single shot"
            )`
@Min-Li Min-Li requested a review from natestemen July 20, 2023 22:22
Copy link
Member

@natestemen natestemen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience Min. This looks good! 🚢

Copy link
Member

@andreamari andreamari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After updating the warning in the function docstring consistently with the new ValueError, this looks good to me!

mitiq/shadows/quantum_processing.py Outdated Show resolved Hide resolved
Co-authored-by: Andrea Mari <andreamari84@gmail.com>
@Min-Li
Copy link
Contributor Author

Min-Li commented Jul 21, 2023

@Misty-W Hey Misty, thanks for your comment
,"My understanding is that tqdm is a status bar, please correct me if I'm missing something. " yes, you are right,
"Is there a particular reason why a status bar is a "must have" for Classical Shadows?" This measurement may contain more than the number of hundred thousand measurements, depending on the number of qubits in the circuit, error rates, failer rate, and observables. This could be quite long progress, and without giving any hints of the progress bar, I am concerned about the user might kill the ongoing progress.
But I have made the tqdm an optional package, even if the user doesn't have it, it would be definitely okay. So we do not need to add it as a dependency.

@Min-Li Min-Li merged commit 536d1a0 into unitaryfund:master Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants