Add map_operations and map_moments transformer primitives
#4692
Conversation
tanujkhattar
changed the title
map_operations and map_moments transformer primitives
There was a problem hiding this comment.
A few high level comments before a more detailed review. My main question is if we need the unroll_circuit_op_*** variants and we can't get away with having just the one function with an extra parameter to change the behavior. Maybe something like:
def unroll_circuit_op(circuit, ..., insert_strategy=None):
"""Unroll circuit operations and emplace them in the outer circuit. if insert_strategy=None a greedy frontier insertion is done."""or something
|
|
||
| Args: | ||
| circuit: Input circuit to apply the transformations on. The input circuit is not mutated. | ||
| map_func: Mapping function from (cirq.Operation, moment_index) to a cirq.OP_TREE. If the |
There was a problem hiding this comment.
Why is the index i needed ?
There was a problem hiding this comment.
The index i is needed for situations when the mapping to be applied on the operation also depends upon which moment the operation is currently present in. For example, when characterizing and compensating for fsim gate angles, the moment index can be useful to determine what compensation to apply.
| def map_operations_and_unroll( | ||
| circuit: CIRCUIT_TYPE, | ||
| map_func: Callable[[ops.Operation, int], ops.OP_TREE], | ||
| ) -> CIRCUIT_TYPE: | ||
| """Applies local transformations via `cirq.map_operations` & unrolls intermediate circuit ops. | ||
|
|
||
| See `cirq.map_operations` and `cirq.unroll_circuit_op` for more details. | ||
|
|
||
| Args: | ||
| circuit: Input circuit to apply the transformations on. The input circuit is not mutated. | ||
| map_func: Mapping function from (cirq.Operation, moment_index) to a cirq.OP_TREE. | ||
| """ | ||
| return unroll_circuit_op(map_operations(circuit, map_func)) |
There was a problem hiding this comment.
Is this really needed ?
There was a problem hiding this comment.
The map_operations_and_unroll method? It's expected to be the most common use case of these two primitives and hence the convenience method will reduce 2 function calls to 1 function call at all call sites.
| ) | ||
|
|
||
|
|
||
| def unroll_circuit_op( |
There was a problem hiding this comment.
Can we merge these three unroll_circuit_op_*** into one ?
There was a problem hiding this comment.
I don't think there's a clean way to merge the unrol_circuit_op** into a single method because 2/3 implementations do not correspond to a standard insert strategy and hence the insert strategy parameter would be misleading at best. We can come up with another enum and pass it as a parameter but I don't see the advantage, especially because the implementations of the 3 methods are also very different.
|
@MichaelBroughton Addressed the comments. Please take another look. Tl;Dr - We cannot combine unroll_circuit_op** methods into one in a clean way and I don't see much advantage in going out of the way to do it. |
…mlib#4692) * Add map_operations and map_moments transformer primitives * Improve circuit type conversion efficiency * Add return entries to docstrings
…mlib#4692) * Add map_operations and map_moments transformer primitives * Improve circuit type conversion efficiency * Add return entries to docstrings
4 participants
The PR introduces new transformer primitives --
map_operationsandmap_moments, as proposed in https://tinyurl.com/moment-preserving-transformers.Part of roadmap item #3237 and #4483