Should we make circuit._prev_moment_available given impact of CCOs ?

[tanujkhattar]

Description of the issue
While editing circuits, we often used circuit.prev_moment_operating_on(op.qubits, end_moment_index) to identify the position after which operation op can be inserted. This, however, is no longer correct and can lead to subtle errors, like the one came up during review of #4944

For ex:

>>> import cirq
>>> q = cirq.LineQubit.range(3)
>>> c = cirq.Circuit(
>>>     cirq.measure(q[0], key='m'),
>>>     cirq.X(q[1]).with_classical_controls('m'),
>>>     cirq.Moment(cirq.X.on_each(q[1], q[2])),
>>> )
>>> print(c)
0: ───M───────────
      ║
1: ───╫───X───X───
      ║   ║
2: ───╫───╫───X───
      ║   ║
m: ═══@═══^═══════
>>> op = c[1][q[1]]
>>> c.prev_moment_operating_on(op.qubits, 1) # Returns None, implies 0 as valid position to insert.

>>> c._prev_moment_available(op, 1) # Returns 1 as the valid position to insert.
1

We should either update (add a flag?) to prev_moment_operating_on s.t. it takes care of CCOs or make _prev_moment_available a public method.

cc @95-martin-orion @daxfohl

Cirq version
0.14dev

[tanujkhattar]

As a stretch goal, we should also find all existing usages of prev_moment_operating_on and figure out whether it breaks with CCOs and needs to be updated to use _prev_moment_available.

[tanujkhattar]

To my absolute surprise, we only have a single usage of prev_moment_operating_on which I added recently in the merge_operations transformer primitive. i.e.

>>> grep -RI 'prev_moment_operating_on' [^.]*/** | grep -v '_test.py'
cirq-core/cirq/circuits/circuit.py:    *   prev_moment_operating_on
cirq-core/cirq/circuits/circuit.py:    def prev_moment_operating_on(
cirq-core/cirq/circuits/circuit.py:    *   prev_moment_operating_on
cirq-core/cirq/optimizers/synchronize_terminal_measurements.py:            moment_index = cast(int, circuit.prev_moment_operating_on((qubit,)))
cirq-core/cirq/transformers/transformer_primitives.py:            idx = ret_circuit.prev_moment_operating_on(tuple(op_qs))
cirq-core/cirq/transformers/transformer_primitives.py:                idx = ret_circuit.prev_moment_operating_on(tuple(op_qs))

Filed #4977 to investigate and fix breakages in merge_operations.

[95-martin-orion]

I like making _prev_moment_available public, but I'm concerned that taking an operation as input is too limiting - the function really only needs the set of qubits and measurement keys to check for. Whatever can manage this with the least complex deprecation process should be fine.

[tanujkhattar]

Can you give an example where taking an operation as input might be too limiting? Circuits only support inserting operations and when asking the previous moment available for insertion, it seems natural to me to take an operation as input.

[95-martin-orion]

Can you give an example where taking an operation as input might be too limiting?

This is a fair point - all cases I can come up with eventually want to add an operation anyways, so just using that operation for the check should be fine.

The main reason I raised this is that an operation is strictly more information than the method needs. Taking an operation forces creating a dummy op (e.g. cirq.measure(*qubits, key='k')) if there ever is a case where there isn't a relevant operation to use.

[tanujkhattar]

an operation is strictly more information than the method needs

I agree. Maybe we can make this public as it is now and go through a deprecation cycle once we identify need for cases where an operation is not needed?