Skip to content

Conversation

@ddddddanni
Copy link
Collaborator

This PR does the following two things:

  1. Improved Readout Benchmarking: Modified run_shuffled_with_readout_benchmarking to accept a list of qubit tuples (List[Tuple[Qubit]]). This allows for calculating the readout error rate on specific qubit subsets, ensuring alignment with the qubit sets used in non-identity Pauli operators within Pauli strings.

  2. Added a tool to compute expectation values for Pauli operators. This tool calculates both unmitigated and readout-error-mitigated expectation values for each circuit, and integrates the results from the readout benchmarking.

@ddddddanni ddddddanni requested review from a team and vtomole as code owners February 14, 2025 00:41
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Feb 14, 2025
@ddddddanni
Copy link
Collaborator Author

Hey @eliottrosenberg and @NoureldinYosri if you could take a look at this. Thanks!

@NoureldinYosri NoureldinYosri self-requested a review February 14, 2025 01:46
@NoureldinYosri NoureldinYosri self-assigned this Feb 14, 2025
Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

Very nice work Danni!! This looks really great!

A few initial comments:

Regarding line 55 in shuffle_circuits_with_readout_benchmarking.py, it should be possible to specify 0 for num_random_bitstrings in order to do the measurement without readout mitigation (or have a different way of running without readout mitigation).

I think you went with lists of tuples for circuits_to_pauli because you can't put a cirq.Circuit into a dictionary, but you can put a cirq.FrozenCircuit into a dictionary, so maybe that would be more convenient (not sure). Also, the object that measure_pauli_strings outputs is pretty complicated, with lots of nested tuples, so maybe we want to create some data class for the output?

…_random_bitstrings, and add a test to cover the situation. The design is the run_shuffled_with_readout_benchmarking method will return a empty SingleQubitReadoutCalibrationResult if no calibration is actually done.

2. Allow measure_pauli_strings to take num_random_bitstrings = 0. In this case, no mitigation is actually done, and the mitigated result and unmitigated result are the same. Also add a test to handle this situation.
3. Make the return type of measure_pauli_strings a data class.
@ddddddanni
Copy link
Collaborator Author

Very nice work Danni!! This looks really great!

A few initial comments:

Regarding line 55 in shuffle_circuits_with_readout_benchmarking.py, it should be possible to specify 0 for num_random_bitstrings in order to do the measurement without readout mitigation (or have a different way of running without readout mitigation).

I think you went with lists of tuples for circuits_to_pauli because you can't put a cirq.Circuit into a dictionary, but you can put a cirq.FrozenCircuit into a dictionary, so maybe that would be more convenient (not sure). Also, the object that measure_pauli_strings outputs is pretty complicated, with lots of nested tuples, so maybe we want to create some data class for the output?

Done.
In the new commit, I allow shuffle_circuits_with_readout_benchmarking to take 0 for num_random_bitstrings, and add a test to cover the situation. The design is the run_shuffled_with_readout_benchmarking method will return a empty SingleQubitReadoutCalibrationResult if no calibration is actually done.
I also allow measure_pauli_strings to take num_random_bitstrings = 0. In this case, no mitigation is actually done, and the mitigated result and unmitigated result are the same. Also add a test to handle this situation. (not sure if we really want to have this situation supported?)
Besides, I make the return type of measure_pauli_strings a data class CircuitToPauliStringsMeasurementResult. Thanks for the suggestion, the code is more readable with this change.

Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

Thanks, Danni!! I tried testing it in this colab and got the error shown there.

@ddddddanni
Copy link
Collaborator Author

Thanks, Danni!! I tried testing it in this colab and got the error shown there.

Can you try again? The problem should be fixed in the latest commit ;)

Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

Thanks, Danni! I tested it again, and there seems to be a bug in the way it is computing the expectation value. I tried it on a simulator without noise in a case where the expectation value should be 1 and got something close to 0. https://colab.sandbox.google.com/drive/1_on4xIHQNMH_2km3RjlfoZxBcYim9A3y

Let me know if you have any trouble identifying the cause of this and I can step in and help.

@eliottrosenberg
Copy link
Collaborator

@ddddddanni
Copy link
Collaborator Author

@ddddddanni Here is a simpler example that illustrates the bug: https://colab.sandbox.google.com/drive/1DUOEbrJSHIvlIN1CI7T6lug4zGDB5nyG

Thanks for catching this! This is due to the code doesn't cover the situation that some pauli strings could be pauli I. I will fix it.

Copy link
Collaborator

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

@ddddddanni LGTM ... I will take another look after you fix the failing CIs

from cirq.study import ResultDict


@dataclasses.dataclass
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer to use attrs over dataclasses

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

elif pauli_op == ops.Y:
# Rotate to Y basis: Rx(pi/2)
operations.append(ops.rx(np.pi / 2)(qid_list[qubit_index]))
# No operation needed for Pauli Z or I (identity)
Copy link
Collaborator

Choose a reason for hiding this comment

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

there shouldn't be identity in this case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

# Extract unique qubit tuples from all circuits
unique_qubit_tuples = set()
for circuit in circuits_to_pauli.keys():
unique_qubit_tuples.add(tuple(sorted(set(circuit.all_qubits()))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

circuit.all_qubits() returns a frozenset, so you can dop the set here and elsewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure!

@ddddddanni
Copy link
Collaborator Author

@ddddddanni Here is a simpler example that illustrates the bug: https://colab.sandbox.google.com/drive/1DUOEbrJSHIvlIN1CI7T6lug4zGDB5nyG

Done! Can you try test again? The issue was Pauli I was incorrectly treated as Z in expectation calculation, and now the issue is fixed, and I also modified tests to test this.

Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

Thanks, @ddddddanni! This is really great! One more request: can we multiply the result of measuring the PauliString by its coefficient? For example, right now your code does not distinguish between the following PauliStrings:

cirq.PauliString(cirq.Z.on_each(cirq.LineQubit.range(2)))

and

-1 * cirq.PauliString(cirq.Z.on_each(cirq.LineQubit.range(2)))

@eliottrosenberg
Copy link
Collaborator

(And when you implement my previous suggestion, make sure to also scale the statistical uncertainty by abs(coefficient))

…ng by its coefficient 2. Added a function to validate the input. Also added some tests for the validation function. 3. Fixed lint
@ddddddanni
Copy link
Collaborator Author

Thanks, @ddddddanni! This is really great! One more request: can we multiply the result of measuring the PauliString by its coefficient? For example, right now your code does not distinguish between the following PauliStrings:

cirq.PauliString(cirq.Z.on_each(cirq.LineQubit.range(2)))

and

-1 * cirq.PauliString(cirq.Z.on_each(cirq.LineQubit.range(2)))

Sure! I added some codes to multiple the coefficient with the result of measuring the paulistring. Also added some more codes to validate the input and tests.

Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

Thanks, @ddddddanni, this is great!

…o validate the input pauli coefficient must be real.
@ddddddanni
Copy link
Collaborator Author

@ddddddanni LGTM ... I will take another look after you fix the failing CIs

Thanks! Failing checks should be fixed now ;)

@codecov
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.14%. Comparing base (eddb281) to head (1af26c2).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7067    +/-   ##
========================================
  Coverage   98.14%   98.14%            
========================================
  Files        1098     1100     +2     
  Lines       95816    96186   +370     
========================================
+ Hits        94034    94403   +369     
- Misses       1782     1783     +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

LGTM%style comments

from cirq.study import ResultDict


@attrs.define
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is intended to be an immutable class prefere @attrs.frozen else prefer @attrs.mutable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, changed it to @attrs.frozen

mitigated_stddev: float
unmitigated_expectation: float
unmitigated_stddev: float
calibration_result: Optional[SingleQubitReadoutCalibrationResult]
Copy link
Collaborator

Choose a reason for hiding this comment

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

since this is optional, it should have a default value = None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

calibration_result: Optional[SingleQubitReadoutCalibrationResult]


@attrs.define
Copy link
Collaborator

Choose a reason for hiding this comment

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

SAME HERE

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!


def _validate_input(
circuits_to_pauli: Dict[circuits.FrozenCircuit, list[ops.PauliString]],
rng_or_seed: Union[np.random.Generator, int],
Copy link
Collaborator

Choose a reason for hiding this comment

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

in most scientific libraries, the rng comes last

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! Moved this to the end.

if qubit in pauli_string:
pauli_op = pauli_string[qubit]
if pauli_op == ops.X:
operations.append(ops.ry(-np.pi / 2)(qubit))
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add a comment that this is hadamard?

Suggested change
operations.append(ops.ry(-np.pi / 2)(qubit))
operations.append(ops.ry(-np.pi / 2)(qubit)) # =cirq.H

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

"""Builds a list of empty confusion matrices"""
cms: list[np.ndarray] = []
for _ in range(qubits_length):
cms.append(_build_one_qubit_confusion_matrix(0, 0))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is is not an empty matrix ... if the intention is to create a placeholder list of np.array then you can do

return [np.empty(0)]*qubits_length

if you need them to be 2x2 array, then

return [np.empty((2,2))]*qubits_length

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! I changed the method to directly return the [_build_one_qubit_confusion_matrix(0, 0) for _ in range(qubits_length)]

Returns:
A list of PauliStringMeasurementResult objects, where each object contains:
- The Pauli string that was measured.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is redundant since this description should be in the docstring of PauliStringMeasurementResult

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! Changed.

def measure_pauli_strings(
circuits_to_pauli: Dict[circuits.FrozenCircuit, list[ops.PauliString]],
sampler: work.Sampler,
rng_or_seed: Union[np.random.Generator, int],
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

readout_qubits = [qubits_1, qubits_2]

# Generate random input circuits and append measurements
input_circuit_1 = rqcg.generate_library_of_2q_circuits(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should create a method to create the input_circuits and use it here and elsewhere

input_circuits = _create_test_circuits()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

circuit_3 = cirq.FrozenCircuit(_create_ghz(8, qubits_3))

circuits_to_pauli: Dict[cirq.FrozenCircuit, list[cirq.PauliString]] = {}
circuits_to_pauli[circuit_1] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know about the tradeoff between damp and dry tests and to me this feels a bit too damp ... maybe

circuits_to_pauli[circuit_1] = [_generate_random_pauli_string(qubits_1, True) for _ in range(3)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@NoureldinYosri
Copy link
Collaborator

great work @ddddddanni ... please fix the coverage CI and then we can merge this PR

@ddddddanni
Copy link
Collaborator Author

great work @ddddddanni ... please fix the coverage CI and then we can merge this PR

Thanks!! The coverage check should be fixed now ; )

@NoureldinYosri NoureldinYosri added this pull request to the merge queue Mar 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2025
@NoureldinYosri NoureldinYosri added this pull request to the merge queue Mar 25, 2025
Merged via the queue into quantumlib:main with commit b3561b4 Mar 25, 2025
38 checks passed
BichengYing pushed a commit to BichengYing/Cirq that referenced this pull request Jun 20, 2025
…dout error mitigation (quantumlib#7067)

* Modify the shuffle_circuit measurement and allow it to handle a list of qubits as input

* Add a new tool for measuring expectation values of Pauli strings with readout error mitigation

* Allow the measure_pauli_strings to also return the SingleQubitReadoutCalibrationResult. Besides, fixed some lints

* 1. Allow shuffle_circuits_with_readout_benchmarking to take 0 for num_random_bitstrings, and add a test to cover the situation. The design is the run_shuffled_with_readout_benchmarking method will return a empty SingleQubitReadoutCalibrationResult if no calibration is actually done.
2. Allow measure_pauli_strings to take num_random_bitstrings = 0. In this case, no mitigation is actually done, and the mitigated result and unmitigated result are the same. Also add a test to handle this situation.
3. Make the return type of measure_pauli_strings a data class.

* Fix a issue that caused calibration_results lookup failure

* Fix: Pauli I was incorrectly treated as Z in expectation calculation

* 1. Added some codes to multiply the result of measuring the PauliString by its coefficient 2. Added a function to validate the input. Also added some tests for the validation function. 3. Fixed lint

* Fix a broken test

* pauli_string.qubits returns the self.keys which are already unique. Thus remove the set() acts on the pauli_string.qubits

* Fix type and coverage check. Besides, adds a input validation check to validate the input pauli coefficient must be real.

* Address comments by @NoureldinYosri

* Fix the coverage check

* Fix lint line too long

---------

Co-authored-by: eliottrosenberg <[email protected]>
Co-authored-by: Noureldin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: L 250< lines changed <1000

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants