-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Moment.from_ops
to more efficiently construct moments
#6078
Conversation
+1 from ops. I've hacked in my own constructor in the past for performance reasons; would be nice to have this. The single letter things seem not very readable. Even if I grant that you can tell |
Agree with Matt. I think the from_ops looks fine to me, but I don't think we should include the one-letter short-hand. It's too ambiguous and having the short-hand bypass validation seems risky. |
+1 to everything said above by Doug and Matt. If you want the shorthand to bypass validation, I'm fine with |
cc = CirqVerbatimBuilder()
circuit = cc.c([
cc.m([cirq.H]),
cc.m(ops),
]) c = CirqVerbatimBuilderWithOperatorOverrides()
# use `__call__` and `__item__`
circuit = c([
c[cirq.H, cirq.Z],
c[ops]
]) |
40a8e3a
to
72ed1eb
Compare
Thanks for the comments, everyone. I've stripped out the |
I agree with this in principle. Unfortunately we tried to cram a lot much flexibility into the constructors of basic data structures like |
@@ -80,17 +81,26 @@ class Moment: | |||
are no such operations, returns an empty Moment. | |||
""" | |||
|
|||
def __init__(self, *contents: 'cirq.OP_TREE') -> None: | |||
def __init__(self, *contents: 'cirq.OP_TREE', _flatten_contents: bool = True) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update the serialization to also include this flag? If not, deserialization would still be inefficient even if the serialized list of operations is flat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done.
yeah, I likewise agree that we messed up our constructors :/ |
@@ -106,6 +116,10 @@ def __init__(self, *contents: 'cirq.OP_TREE') -> None: | |||
self._measurement_key_objs: Optional[FrozenSet['cirq.MeasurementKey']] = None | |||
self._control_keys: Optional[FrozenSet['cirq.MeasurementKey']] = None | |||
|
|||
@classmethod | |||
def from_ops(cls, *ops: 'cirq.Operation') -> 'cirq.Moment': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc that this doesn't do any flattening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added docstring.
Moment.from_ops
to more efficiently construct moments
This adds a new classmethod
cirq.Moment.from_ops
for constructing a moment from operations without callingflatten_to_ops
. This can save a significant amount of time in workloads that construct a lot of circuits. For example, in one internal benchmark we spend almost 3% of the total runtime in thecirq.Moment
construct (20s out of 676s total).This also adds functions
cirq.m
andcirq.c
as shorthands forcirq.Moment.from_ops
andcirq.FrozenCircuit.from_moments
, repsectively. These are similar to thecirq.q
helper for constructing qubit types (#5181) while trying to guide users to use cirq in the most performant way: avoiding theOP_TREE
overhead associated with the default moment and circuit constructors, and preferringFrozenCircuit
which is immutable and so can cache computed properties of the circuit. These single-letter aliases may be more controversial, but I thought I'd at least propose them. They could also be split out from this PR because I thinkcirq.Moment.from_ops
is useful even without the helper functions.