Rewrite cirq.stratified_circuit following new Transformer API and primitives.
#4944
Conversation
There was a problem hiding this comment.
Looks great. Took a quick look around and this transformer doesn't seem to get heavy use in the library (it also has exponential runtime in the number of categories).
Should we just cut it outright ? If not, I've left a few comments and we should be good to merge.
| Args: | ||
| circuit: The circuit whose operations should be re-arranged. | ||
| context: `cirq.TransformerContext` storing common configurable options for transformers. | ||
| categories: A list of classifiers picking out certain operations. |
There was a problem hiding this comment.
Could we tighten this up to reflect the types that you are ultimately allowed to pass in here ? It seems like this is always getting promoted to a lambda function of some kind in the _category_to_classifier to function, maybe we just require lambdas from the get go ?
There was a problem hiding this comment.
We already describe all the valid types in the description of this arg?
There are several ways to specify a classifier. You can pass in a gate instance
(e.g. `cirq.X`), a gate type (e.g. `cirq.XPowGate`), an operation instance
(e.g. `cirq.X(cirq.LineQubit(0))`), an operation type (e.g.`cirq.CircuitOperation`),
or an arbitrary operation predicate (e.g. `lambda op: len(op.qubits) == 2`).
I think it's useful to allow these types instead of directly requiring lambda's because an iterable of lambda's would be messy to write. For example:
>>> categories = [cirq.X, cirq.Y, cirq.Z] # Way-1: Much cleaner
>>> categories = [lambda op: isinstance(op.gate, cirq.X), lambda op: isinstance(op.gate, cirq.Y), lambda op: isinstance(op.gate, cirq.Z)] # Way-2: Much worse | assert cirq.stratified_circuit(input_circuit, categories=[cirq.X]) == expected | ||
| cirq.testing.assert_same_circuits( | ||
| cirq.stratified_circuit(input_circuit, categories=[cirq.X]), expected | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Maybe add a CCO test or two just so we have it and know what happens and then when we get around to well-defining their behavior in transformers, we know where we are starting from ?
There was a problem hiding this comment.
CCOs are indeed tricky! Fixed a bug, added a test and filed #4976 -- which I'm sure is going to be a cause of bugs with CCOs at a lot more places in cirq :))
…tify_transformer
…r/Cirq into stratify_transformer
The number of categories in practice are usually pretty small, for example: align all 1 and 2q gates in separate moments. Therefore, I think this is a useful transformer to keep. I've addressed your comments. PTAL. |
|
Could you try pushing a no-op to the branch here to retrigger the CI ? I can't seem to get it to go..... |
…rimitives. (quantumlib#4944) * Rewrite stratified_circuit following new Transformer API and primitives. * Reorder arguments lists * Address Mike's Feedback
…rimitives. (quantumlib#4944) * Rewrite stratified_circuit following new Transformer API and primitives. * Reorder arguments lists * Address Mike's Feedback
…rimitives. (quantumlib#4944) * Rewrite stratified_circuit following new Transformer API and primitives. * Reorder arguments lists * Address Mike's Feedback
cirq/optimizers/stratify.pytocirq/transformers/stratify.py.