single_qubit_state_tomography issues with multiple key measurements

[kygnz]

Issue:

When trying to run single_qubit_state_tomography on a CircuitOperation object that contains measurements, my group and I run into the error "cannot extract 2D measurements for repeated keys". Upon observing the file qubit_characterizations.py, we found the issue lies in the function definition of single_qubit_state_tomography, with rho_11, rho_01_im, and rho_01_re initialized using results.measurements. Changing the lines to results.records eliminates the error. Is this an unintended bug in the tomography function?

How to recreate the issue:

import cirq
import cirq.experiments
import sympy

qubits = cirq.LineQubit.range(3)
circuit = cirq.Circuit()

circuit.append(
    [
        cirq.X(qubits[0]),
        cirq.X(qubits[1]),
        cirq.X(qubits[2]),
    ]
)


circuit.append(
    [
        cirq.measure(qubits[0], key="a"),
        cirq.measure(qubits[1], key="b"),
        cirq.measure(qubits[2], key="c"),
    ]
)

a, b, c = sympy.symbols("a,b,c")
sympy_cond = cirq.SympyCondition(sympy.Eq(a + b + c, 0))


print(circuit)
sim = cirq.Simulator()

circuitBuilt = cirq.Circuit(
    cirq.CircuitOperation(
        circuit=circuit.freeze(),
        repeat_until=sympy_cond,
        use_repetition_ids=False,
    )
)

print("\n------------------- single qubit state tomography -------------------")
# Without updating qubit_characterizations.py, tomography_result produces an error
tomography_result = cirq.experiments.single_qubit_state_tomography(
    sim, qubits[0], circuitBuilt, 1000
)

print(tomography_result.data)

Cirq Version:

1.5.0

[dstrain115]

Discussion from cirq cync. This issue is asking for the single qubit tomography experiment to support circuits where there are intermediate measurements. This seems like it would be a reasonable request, but we should clarify in the documentation how this works, and what it is doing on the tomography on in this case where there are repeated measurements.

[mhucka]

@kygnz Can you confirm that Doug's explanation is your intention?

We'd also appreciate any other elaboration and details you can provide about what you're trying to do and how you ran into this.

[daxfohl]

Historical context: single_qubit_state_tomography was written years before Result.record field was created to support subcircuits with loops (or more specifically, circuits with multiple measurements to the same key). I'm not familiar with tomography, but from a quick look at the function, it only reads the z key, which it manually appends to the circuit so there will only be one measurement to that key, so it seems like changing measurements to records there would be the correct fix: it doesn't matter that other keys are repeated; all it cares about is the final z. (Though it should also probably handle the case where the circuit already defines a z key).

Until then as workaround, I see your circuit sets use_repetition_ids=False. I believe if you set use_repetition_ids=True, that will cause each measurement of the loop to go to a separate key, which I'm pretty sure won't fail when single_qubit_state_tomography calls result.measurements under the hood.

Update on workaround: oh wait, does repeat_until require use_repetition_ids=False? IIRC it might. If so, maybe that's something that could be improved. The subcircuit should be able to inject the current repetition id when it's testing the repeat_until condition (if it doesn't already; can't remember).

[kygnz]

@mhucka Yes, Doug's explanation captures the core of the problem! A bit of context, my team and I are implementing the purification algorithm for distilling |M〉as found here
and we want to use tomography on our resulting state to confirm we are implementing it correctly (the example snippet I provided is a simple recreation of the issue, not our full code)

[kygnz]

@daxfohl Thanks for the context! Yes, for our circuit when I don't set the repetition ids parameter to False I get the error: ValueError: Cannot unroll circuit due to nondeterministic repetitions

I don't mind making the fix if no one has claimed/been assigned to it yet!

[mhucka]

@kygnz Thank you for your interest and willingness to take this on. I've assigned the issue to you.

[daxfohl]

@kygnz yeah if you want to give it a look. I don't have my home laptop in front of me so I'm not able to run the code. But https://github.com/quantumlib/Cirq/blob/8788b625da0a57459589d2377a27bdd0250035c3/cirq-core/cirq/circuits/circuit_operation.py#L444 in CircuitOperation._act_on_ is where the loop condition gets checked. If repetition keys are enabled though, the condition would have to be massaged to look for keys a_1, b_1, c_1, a_2, b_2, c_2, etc, incrementing with each repetition, instead of the original keys a, b, c. (Note, some care needs to be taken because some keys in the condition may be from somewhere earlier in the circuit before the loop. So only the keys that are inside the loop should be incremented like that.)

Also, just looking at the function that throws that exception, it looks like it is not called from _act_on_, so it must be called from some other method prior to the actual simulation execution. There may be a reason for that that could be hard to fix. If you can figure it out, great. if not, I'll give it a look when I get a chance.

---

update: @kygnz clarification: the above comment is for if you were saying you wanted to fix the CircuitOperation to allow repeat_until and repetition_ids simultaneously. If you were just saying that you wanted to fix the measurements->records you can ignore it. I looked a bit closer at the CircuitOperation code and it looks like it'd be pretty tricky to fit in, and I don't think it'd add much value, so I'd probably avoid it.

One thing to consider if you want to go slightly beyond changing measurements to records in the tomography function (purely optional): if a circuit has multiple measurements to the same key, it might make sense for Result.measurements to return the final measurement for that key, rather than throwing an exception. I think this would improve usability and preemptively fix any other bugs hanging around that use measurements instead of records. @maffoo WDYT?

[kygnz]

While we wait for @maffoo to respond, I can make a pull request and make the first fix of changing measurements to records

[maffoo]

I'm not particularly keen on the idea of changing Result.measurements to return the values for the last instance of a key, because I feel like that could mask other problems. If you use a key more than once then you should think about those multiple instances when analyzing the results. If you want the last instance you can get that data with result.records[key][:, -1, :], and having result.measurements[key] fail in this case is I think a good thing.

For state tomography, I think we should have the tomo code pick a unique measurement key name that does not conflict with any keys used in the circuit, which should avoid the problem entirely. I commented along these lines on #7421.

[daxfohl]

I think we should have the tomo code pick a unique measurement key name

@maffoo that's what it does already. z is added to the end of the circuit by the tomography library function; it's not part of the original circuit. So z is unique. But the issue is that result.measurements['z'] fails anyway because result.measurments fails whenever any key is not unique. Here, a,b,c are measured multiple times, but never z.

That said, changing it to result.records['z'][:, -1, :] should fix the problem.

[maffoo]

Thanks for the clarification, @daxfohl. I don't think z is a particularly unique name and the tomography code doesn't check that the key is not present in the circuit already, so I do think it would be worth modifying that code to ensure that the key used for tomography is unique. We could also consider modifying the Result.measurements property to fail only if one tries to access a key with multiple instances, but work for other keys.

[daxfohl]

I think your original proposal is actually the best option. Changing the tomography function to use result.records['z'][:, -1, :] is a simple change and works even if a different z is defined elsewhere in the circuit. Modifying Result.measurements is more invasive and still requires updating the tomography function to ensure it uses a unique key that's not present in the circuit.

@kygnz I think just update your PR to explicitly index for the final z measurement only, using result.records['z'][:, -1, :], and it should be good to go.

[kygnz]

Got it, thanks @maffoo and @daxfohl for the insight, I'll go ahead and update the PR later today

[daxfohl]

Fixed by #7477