-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Change switch_to_new condition in TwoQubitCompilationTargetGateset to switch only when it's optimal in terms of 2q gate counts
#5405
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
Conversation
… switch only when it's optimal in terms of 2q gate counts
| any( | ||
| protocols.num_qubits(op) == 2 and op not in self | ||
| for op in ops.flatten_to_ops(old_optree) | ||
| ) |
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.
Can we do this part of the check before constructing new_optree so we don't waste time on it if it's not needed?
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.
We will have to construct the new_optree irrespective of whether this check returns a True or False.
We will not switch to the new_optree only if both the checks are false (i.e. all 2q operations in old_optree are in self and old_2q_gate_count <= new_2q_gate_count.
| return new_optree if switch_to_new else old_optree | ||
| if switch_to_new: | ||
| return new_optree | ||
| mapped_old_optree: List['cirq.OP_TREE'] = [] |
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.
This is different from the old behavior: if all old ops (1q or 2q) are in the target gateset and new_2q_gate_count >= old_2q_gate_count, this previously would have returned the old optree. Is this intended?
(If gateset behavior would prevent this case from occurring, a comment explaining this would be helpful)
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.
this previously would have returned the old optree.
The behavior is unchanged, i.e., if all old ops (1q or 2q) are in the target gateset and new_2q_gate_count >= old_2q_gate_count; it would have previously returned old_optree and it would still return old_optree.
Notice that the mapped_old_optree will be same as old_optree if all operations are in self (the first if condition in the loop below).
… switch only when it's optimal in terms of 2q gate counts (quantumlib#5405)
… switch only when it's optimal in terms of 2q gate counts (quantumlib#5405)
… switch only when it's optimal in terms of 2q gate counts (quantumlib#5405)
cirq.TwoQubitCompilationTargetGatesetcurrently implements a logic to compare the analytical decomposition of a merged 2q connected component (new_optree) with the one which already exists in the circuit (old_optree) and switch iff:old_optreeis not part of the target gatesetnew_optreeis less than those inold_optreeThis PR changes the switch condition to:
old_optreeis not part of the target gateset # Any ==> Any 2qnew_optreeis less than those inold_optreeThis is useful because if the old merged connected component (i.e.
old_optree) is optimal in terms of 2q gates but contains single qubit gates outside of the target gateset; we should only replace the single qubit gates and not the entire decomposition with a more in-efficient decomposition.See the newly added test for a dummy example.
cc @verult ; Part of #5254