Skip to content
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

AbstractCircuit.freeze should not reallocate moments #5816

Closed
maffoo opened this issue Aug 4, 2022 · 2 comments · Fixed by #5878
Closed

AbstractCircuit.freeze should not reallocate moments #5816

maffoo opened this issue Aug 4, 2022 · 2 comments · Fixed by #5878
Assignees
Labels
area/circuits good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/bug-report Something doesn't seem to work. triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add

Comments

@maffoo
Copy link
Contributor

maffoo commented Aug 4, 2022

Description of the issue

When freezing circuits, Moment instances should be reused since they are immutable. This was previously the case, but seems to have changed recently (possibly due to #5332).

How to reproduce the issue

In [1]: q = cirq.q(0)

In [2]: c = cirq.Circuit(cirq.X(q), cirq.measure(q))

In [3]: f = c.freeze()

In [4]: [mc is fc for mc, fc in zip(c, f)]
Out[4]: [False, False]

Cirq version

1.1.0.dev20220801180412

@maffoo maffoo added the kind/bug-report Something doesn't seem to work. label Aug 4, 2022
@maffoo maffoo self-assigned this Aug 4, 2022
@daxfohl
Copy link
Contributor

daxfohl commented Aug 5, 2022

Yes, looks like it was from #5332. The with_operations below is what does it. Moment.with_operations creates a new Moment even if the input list is empty.

self._moments.append(moments_by_index[i].with_operations(op_lists_by_index[i]))

You could either change the code here to skip the with_operations call if there are no operations to append, or alternatively, have Moment.with_operations return self if the input is empty.

@vtomole vtomole added the triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add label Aug 10, 2022
@verult
Copy link
Collaborator

verult commented Aug 10, 2022

From Cirq sync - this will be considered a bug fix as it returns the API behavior to the original intention.

@viathor viathor added triage/discuss Needs decision / discussion, bring these up during Cirq Cynque area/circuits labels Aug 30, 2022
@tanujkhattar tanujkhattar added the good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. label Aug 31, 2022
@verult verult removed the triage/discuss Needs decision / discussion, bring these up during Cirq Cynque label Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/circuits good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/bug-report Something doesn't seem to work. triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants