Preserve subcircuits passed to [Frozen]Circuit.from_moments

[maffoo]

Preserving subcircuits is very useful internally because we use subcircuit identity when compiling to hardware to guide waveform reuse. Note that previously if a circuit was passed to from_moments it would be passed to Moment which would fail for any circuit that had multiple moments with gates on the same qubit, because those could not be collapsed into a single moment. Single-moment circuits would've worked in the past and they will continue to work but will now being contained in a circuit operation.

Here's an example of a single-moment circuit:

circuit = Circuit.from_moments(
    FrozenCircuit(Moment(X(q0), X(q1)))
)

# previously:
circuit == Circuit(
    Moment(X(q0), X(q1))
)

# with this PR:
circuit == Circuit(
    Moment(
        CircuitOperation(
            FrozenCircuit(Moment(X(q0), X(q1)))
        )
    )
)

Here's an example of a multi-moment circuit with repeated operations:

circuit = Circuit.from_moments(
    FrozenCircuit(
        Moment(X(q0)),
        Moment(Y(q0)),
    )
)

# previously:
# exception! can't put X(q0) and Y(q0) in a single moment

# with this PR:
circuit == Circuit(
    Moment(
        CircuitOperation(
            FrozenCircuit(
                Moment(X(q0)),
                Moment(Y(q0)),
            )
        )
    )
)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (96b3842) 97.89% compared to head (143ec31) 97.89%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6320   +/-   ##
=======================================
  Coverage   97.89%   97.89%           
=======================================
  Files        1108     1108           
  Lines       96192    96209   +17     
=======================================
+ Hits        94165    94186   +21     
+ Misses       2027     2023    -4     

Files	Coverage Δ
cirq-core/cirq/circuits/circuit.py	98.44% <100.00%> (+0.01%)	⬆️
cirq-core/cirq/circuits/circuit_test.py	99.64% <100.00%> (+<0.01%)	⬆️
cirq-core/cirq/circuits/frozen_circuit_test.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tanujkhattar]

I think this further complicates the circuit construction logic, and we already have many different circuit construction paths. I'd like to understand the motivation better.

Can the user just directly call .to_op() at the call site to achieve the deserived effect? So for your examples:

circuit = Circuit.from_moments(
    FrozenCircuit(Moment(X(q0), X(q1))).to_op()
)

and

circuit = Circuit.from_moments(
    FrozenCircuit(
        Moment(X(q0)),
        Moment(Y(q0)),
    ).to_op()
)

Circuit.from_moments is well defined right now. I don't think we should add more magic and special casing unless absolutely required.

[maffoo]

I think this further complicates the circuit construction logic, and we already have many different circuit construction paths. I'd like to understand the motivation better.

My motivation is to make the common case of dealing with subcircuits easier to write. This came up internally because we're trying to move more internal code to use subcircuits with CircuitOperation and there's quite a bit of noise in the code when doing this.

Can the user just directly call .to_op() at the call site to achieve the deserived effect?

Yes, that is doable, though in most case where this comes up internally we're dealing with circuits that are produced in other places and they may not already be frozen, so you end up doing something like:

cirq.Circuit.from_moments(
    state_prep_circuit.freeze().to_op(),
    main_circuit.freeze().to_op(),
    measure_circuit.freeze().to_op(),
)

This seems to me like unneccesary boilerplate compared to allowing the user to do:

cirq.Circuit.from_moments(
    state_prep_circuit, main_circuit, measure_circuit,
)

Circuit.from_moments is well defined right now. I don't think we should add more magic and special casing unless absolutely required.

The proposed change still results in well-defined behavior for from_moments but of course one can disagree about how "magical" this change is and how useful it would be. I was hoping to motivate this by noting that in most cases currently passing a circuit to from_moments results in an exception (and this is an exception that internal users have encountered when trying to use from_moments) so it's not so much changing existing behavior as allowing from_moments to be used in more situations.

[tanujkhattar]

it's not so much changing existing behavior as allowing from_moments to be used in more situations.

Yes, but this opens a can of worms where we can continue to make incremental updates and again end up in a situation like Circuit.__init__. For example,

cirq.Circuit.from_moments(
    cirq.Circuit(cirq.X(a), cirq.Y(a)), # works, creates a moment with circuit operation.
    [cirq.X(a), cirq.Y(b)],  # works, creates a moment with 2 non overlapping operations.
    [cirq.X(a), cirq.Y(a)],  # still fails, but this could also be wrapped in a circuit op?
    # still fails, because of no auto flattening. This looks like a valid potential use case
    # where we want a moment with a circuit operation on qubit `a` and a Z gate on
    # qubit `b` ?
    [cirq.Circuit(cirq.X(a), cirq.Y(a)), cirq.Z(b)],
)

Instead, I'd suggest we either:

a) Add a wrap_subcircuits: bool = False optional parameter (or create a new helper) to methods like flatten_to_ops_or_moments. This can iterate through the given op-tree and wrap every circuit in a circuit operation.
b) If you don't want to add this layer of indirection, consider adding a new method Circuit.from_moments_or_subcircuits(*moments_or_subcircuits: Union[Moment, AbstractCircuit]) and then essentially do what you are doing here. In this case, the name of the method clearly specifies the intention and avoid any potential ambiguity regarding whether my nested circuits would also be converted to circuit ops or not.

What do you think?

[maffoo]

it's not so much changing existing behavior as allowing from_moments to be used in more situations.

Yes, but this opens a can of worms where we can continue to make incremental updates and again end up in a situation like Circuit.__init__.

I agree we want to avoid adding more logic. I think the thing I'd like to do is preserve structure that already exists in the arguments passed to from_moments. So, moments and circuits are preserved. Anything else is wrapped in a moment. I don't anticipate any more cases than that within the current model of circuits and moments.

For example,

cirq.Circuit.from_moments(
    cirq.Circuit(cirq.X(a), cirq.Y(a)), # works, creates a moment with circuit operation.
    [cirq.X(a), cirq.Y(b)],  # works, creates a moment with 2 non overlapping operations.
    [cirq.X(a), cirq.Y(a)],  # still fails, but this could also be wrapped in a circuit op?

I don't see a reason to wrap this in a circuit op. The goal is to preserve the structure that is present in the args, not to add any new structure (especially not if it would require inspecting the contents beyond checking the type, e.g. traversing into an op tree). If you want X(a) and Y(a) to end up in separate moments, you have to pass them as separate args to from_moments, and if you want them in a subcircuit you have to create the circuit explicitly.

    # still fails, because of no auto flattening. This looks like a valid potential use case
    # where we want a moment with a circuit operation on qubit `a` and a Z gate on
    # qubit `b` ?
    [cirq.Circuit(cirq.X(a), cirq.Y(a)), cirq.Z(b)],
)

Yeah, I don't think we should do any auto-flattening, because that is changing the structure provided by the user. If you want to take a circuit and include its moments in the circuit produced by from_moments, you have to expand it manually (this works now and is something we do internally):

Circuit.from_moments(*circuit_as_moments, circuit_as_subcircuit)

This PR preserves the first behavior of circuit_as_moments where we expand moments into the new circuit, while adding the second possibility of circuit_as_subcircuit where we preserve a circuit but wrap it in CircuitOperation.

Instead, I'd suggest we either:

a) Add a wrap_subcircuits: bool = False optional parameter (or create a new helper) to methods like flatten_to_ops_or_moments. This can iterate through the given op-tree and wrap every circuit in a circuit operation. b) If you don't want to add this layer of indirection, consider adding a new method Circuit.from_moments_or_subcircuits(*moments_or_subcircuits: Union[Moment, AbstractCircuit]) and then essentially do what you are doing here. In this case, the name of the method clearly specifies the intention and avoid any potential ambiguity regarding whether my nested circuits would also be converted to circuit ops or not.

What do you think?

I think a new flag on flatten_to_ops_or_moments would be confusing, but that is basically the behavior that I'd like. I think flatten_to_ops_or_moments should really be flatten_preserving_structure, but we wrote it before CircuitOperation was even a thing, so we didn't anticipate this extra kind of nested structure. I'm also not a big fan of a new method like Circuit.from_moment_or_subcircuits because I think the distinction between that and Circuit.from_moments is rather hard to explain and also because the new behavior applies in situations where Circuit.from_moments can't be used anyway (if you try to pass a subcircuit it will raise an exception rather than produce a different circuit structure).

[tanujkhattar]

Okay, let's get this in and make sure we don't add more special casing to Circuit.from_moments.

We should mark this as a breaking change because cirq.Circuit.from_moments(cirq.Circuit(cirq.X(a), cirq.Y(b))) results in a different behavior now.

Ideally, we should not be doing breaking changes but I think this is a small enough corner case so we can have an exception.

cc @dstrain115 in case you have any concerns.

[maffoo]

Thanks for the careful review, @tanujkhattar! I will keep this open for a while in case other people have comments.