Optimize is_measurement protocol

[smitsanghavi]

Current is_measurement implementation is slow because:

• It delegates to getting all the measurement keys first and then checks for non-emptyness
• Measurement key protocol iterates through each operation in order and decomposes each until it finds _measurement_key_ or _measurement_keys_ implemented.
  • Decomposing can be costly and most of the time would be spent on non-measurement operations.
• There is no early exit. Even if we find a measurement, we aren't declaring victory, we wait to gather all the keys and create the full set. This is disastrous for big circuits with terminal measurements.

Made a few optimizations to move this faster:

• Added _is_measurement_ magic methods on some of the objects for slightly faster evaluation
  • This would give even more benefit if some expensive-to-decompose gates can explicitly set it to False, enabling us to skip it entirely regardless of whether it is decomposable or not.
• Instead of a depth-first-search approach as above (here depth=each level of decompose), we use breadth-first-search to make sure we do all the cheap _is_measurement_ checks before attempting any decompose
• Early escape. As soon as we find a measurement, we declare victory and return.
• Reverse the decomposed. At every "level" we iterate in the reverse order of moments since measurements are typically terminal (or terminal-ish)

This implementation may perform worse than the existing one on circuits that have many many levels of nested CircuitOperations or other composite gates at the start of the circuit and only these ones have measurements. This seems like a pretty non-typical case.

[95-martin-orion]

This is a red flag to me - memoization should not change the public properties of an object.

[smitsanghavi]

True, but _is_measurement_ isn't really a public property. cirq.is_measurement still gives the same answer (line 88) before memoization.
_is_measurement_ is just the first (assumed to be efficient) strategy in calculating the protocol result. It should be allowed to return NotImplemented and let the protocol fallback to alternate strategies (i.e. decomposition).

[95-martin-orion]

See my other comment - not memoizing in _is_measurement_ prevents it from becoming efficient unless the user calls has_measurement, which is inconvenient behavior.

[smitsanghavi]

Makes sense. See my other reply for the caveat in this approach.

[95-martin-orion]

As noted above - if _has_measurements is not set, this should set it.

[smitsanghavi]

Calculating has_measurement from scratch can be very costly. _is_measurement_ cannot be expected to do the full circuit decomposition inside it, the protocol should handle it in the case where _is_measurement_ does not have an efficient answer.

[95-martin-orion]

This is why I'm concerned: for fc = FrozenCircuit(...) if someone invokes cirq.is_measurement(fc), then later invokes fc.has_measurement(), the result will not be memoized for the has_measurement call.

Ideally, something like this would work:

def _is_measurement_(self) -> bool:
    if self._has_measurements is None:
        self._has_measurements = cirq.is_measurement(self.unfreeze())
    return self._has_measurements

Deferring is_measurement handling to the Circuit class prevents infinite recursion in the _is_measurement_from_magic_method step of the protocol.

[smitsanghavi]

Updated to enable memoization. This comes with the downside that a CircuitOperation, once encountered, will now be immediately fully evaluated regardless of how much decomposing it needs. I was aesthetically hesitant to go this route since it deviates from a pure breadth-first search for a measurement. But memoizing the expensive is_measurement call is definitely a net win and worth it.

The is_measurement protocol might slow down a bit for circuits with many non-measurement terminal CircuitOperations, but these will probably be rare.

[95-martin-orion]

Loss of BFS is unfortunate...I have a nagging feeling that there's a clean solution that allows BFS to coexist with proper memoization, but the existing tangle of methods and expected behavior make implementing that solution infeasible :/

[95-martin-orion]

This is why I'm concerned: for fc = FrozenCircuit(...) if someone invokes cirq.is_measurement(fc), then later invokes fc.has_measurement(), the result will not be memoized for the has_measurement call.

Ideally, something like this would work:

def _is_measurement_(self) -> bool:
    if self._has_measurements is None:
        self._has_measurements = cirq.is_measurement(self.unfreeze())
    return self._has_measurements

Deferring is_measurement handling to the Circuit class prevents infinite recursion in the _is_measurement_from_magic_method step of the protocol.

[95-martin-orion]

See my other comment - not memoizing in _is_measurement_ prevents it from becoming efficient unless the user calls has_measurement, which is inconvenient behavior.

[95-martin-orion]

Approval granted, but we should either leave #4141 open or create a follow-up issue to disentangle BFS from memoization.

[CirqBot]

Automerge cancelled: Only collaborators with write permission can use automerge.