Skip to content

Conversation

tanujkhattar
Copy link
Collaborator

Part of #5028

@tanujkhattar tanujkhattar requested review from a team, vtomole and cduck as code owners June 30, 2022 00:22
@tanujkhattar tanujkhattar requested a review from pavoljuhas June 30, 2022 00:22
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CirqBot CirqBot added the size: S 10< lines changed <50 label Jun 30, 2022
@@ -89,6 +89,7 @@ def _interaction(
)


@cirq._compat.deprecated_class(deadline='v0.16', fix="Use cirq.merge_k_qubit_unitaries")
class MergeNQubitGates(cirq.PointOptimizer):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class does more than cirq.merge_k_qubit_unitaries. It calls off to cirq.drop_negligible_operations and cirq.drop_empty_moments as well. Will existing tests for this optimizer pass if only cirq.merge_k_qubit_unitaries is called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to where MergeNQubitGates optimizer calls off to cirq.drop_negligible_operations and cirq.drop_empty_moments ?

Also, the call sites calling MergeNQubitGates have been updated to call cirq.merge_k_qubit_unitaries and no tests fail right now. But to answer your specific question, cirq.merge_k_qubit_unitaries does not call cirq.drop_negligible_operations and cirq.drop_empty_moments by default.

Copy link
Collaborator

@vtomole vtomole Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, i mistook this for simplify_expectation_value_circuit that's after this.

Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 7, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 7, 2022
@CirqBot CirqBot merged commit b592ae7 into master Jul 7, 2022
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 7, 2022
@CirqBot CirqBot deleted the deprecate_merge_n_qubit_gates branch July 7, 2022 01:32
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 7, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants