quantumlib / Cirq

A Python framework for creating, editing, and invoking Noisy Intermediate Scale Quantum (NISQ) circuits.
Apache License 2.0
4.22k stars 1.01k forks source link

`drop_terminal_measurements` fails for measurements on dimension>2 Qids #5994

Open richrines1 opened 1 year ago

richrines1 commented 1 year ago

Description of the issue

the cirq.drop_terminal_measurements transformer fails if any measurements are on Qids with dimension greater than 2

How to reproduce the issue

_ = cirq.drop_terminal_measurements(cirq.Circuit(cirq.measure(cirq.LineQid(0, dimension=2))))  # ok
_ = cirq.drop_terminal_measurements(cirq.Circuit(cirq.measure(cirq.LineQid(0, dimension=3))))  # ValueError: Wrong shape of qids for <cirq.I>. Expected (2,) but got (3,) <[cirq.LineQid(0, dimension=3)]>.

Cirq version

1.2.0.dev20230105212249
richrines1 commented 1 year ago

it looks like this arises when inserting X/I operations according to invert_mask - out of curiosity how should invert_mask be interpreted for qudit measurements?

dstrain115 commented 1 year ago

Adding to triage discuss to answer above question. How should we handle invert_mask when using qudits?

daxfohl commented 1 year ago

To be compatible with deferred_measurement_transformer it has to be equivalent to applying an X on that qudit, e.g. a mod_add(1). (Or if something else is decided, then the deferred_measurement_transformer needs to be updated to match). https://github.com/quantumlib/Cirq/blob/27dd6076f8fb9631c20d6234134edd76c9cdd681/cirq-core/cirq/transformers/measurement_transformers.py#L116

daxfohl commented 1 year ago

I take that back, the deferred_measurement_transformer code doesn't handle it at all. It'd just require changing X to XPowGate and explicitly passing in the dimension though. I think this is the best option nonetheless. If there's a desire to allow int in the invert_mask, for completions sake, that would translate to an exponent in deferred_measurement_transformer's XPowGate.

richrines1 commented 1 year ago

i think that's probably what i'd expect to happen, though also noticing that the simulator currently does something different (applying ^= 1 to the result iff the measured value is either 0 or 1)

also fwiw it looks like the same pattern comes up again in cirq.circuits.circuit._decompose_measurement_inversions, leading to e.g.,

measurement = cirq.measure(cirq.LineQubit(0, dimension=3), invert_mask=(True,))
cirq.has_unitary(cirq.Circuit(measurement))  # ValueError: Wrong shape of qids for <cirq.X>. Expected (2,) but got (3,) <[cirq.LineQid(0, dimension=3)]>.
tanujkhattar commented 1 year ago

invert_mask is supposed to be a classical post processing on the measurement outcomes of a qubit which conveys that we should "flip" the measurement results. As discussed above, extending this to general qids would require us start supporting add_mod(k), so for qutrits one could either add 1 or add 2 (both mod 3) to the measurement results.

However, note that we already support confusion_map parameter in MeasurementGate, which provides a more general way to specify what classical post processing should be applied to the measurement results. Note that confuson_map was introduced after invert_mask and invert_mask has been kept around for legacy and backwards compatibility.

Going forward, I recommend we do the following:

  1. Raise an error for general qudits if an invert_mask is specified.
  2. Use confusion_map to specify the classical post processing that should be applied to general qudits.
  3. Potentially deprecate invert_mask for next major release (cirq 2.0).

@daxfohl @richrines1 Let me know what you think.

daxfohl commented 1 year ago

Makes sense. invert_mask would be a horrible name for an inc_mod function, even though it's the only reasonable qudit behavior. More likely to confuse people than anything.

I think no need to deprecate it though, it's still a useful and meaningful shortcut.

madhurimaparuchuri commented 1 year ago

Hi, is anyone working on this issue, if not I will pick this