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

Create an insertion_sort transformer and incorporate it into the transpiling protocol #6776

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

NoureldinYosri
Copy link
Collaborator

@NoureldinYosri NoureldinYosri commented Oct 16, 2024

Running the compilation workflow with the following command should produce circuits with a very short depth

cirq.optimize_for_target_gateset(circuit, gateset=cirq.CZTargetGateset(preserve_moment_structure=False, reorder_operations=True), max_num_passes=None)

cc: @ikd-sci

fixes #6771

@NoureldinYosri NoureldinYosri requested review from vtomole and a team as code owners October 16, 2024 23:46
@CirqBot CirqBot added the size: S 10< lines changed <50 label Oct 16, 2024
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.84%. Comparing base (81f66b9) to head (a8277a6).

Files with missing lines Patch % Lines
cirq-core/cirq/transformers/insertion_sort.py 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6776      +/-   ##
==========================================
- Coverage   97.84%   97.84%   -0.01%     
==========================================
  Files        1077     1079       +2     
  Lines       92791    92825      +34     
==========================================
+ Hits        90788    90821      +33     
- Misses       2003     2004       +1     

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

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

We need to order by sorted qubits, otherwise LGTM with a few minor comments.

Thanks for adding this!

Comment on lines 42 to 56
sorted_operations: List['cirq.Operation'] = []
for op in circuit.all_operations():
sorted_operations.append(op)
j = len(sorted_operations) - 1
while (
j
and _id(sorted_operations[j]) < _id(sorted_operations[j - 1])
and protocols.commutes(sorted_operations[j], sorted_operations[j - 1], default=False)
):
sorted_operations[j], sorted_operations[j - 1] = (
sorted_operations[j - 1],
sorted_operations[j],
)
j -= 1
return circuits.Circuit(sorted_operations)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please reconsider the implementation I suggested in #6776 (comment). Unlike here, the op.qubits are sorted just once and cached, and the output list is modified just once per iteration - after finding the correct insertion position.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

please consider the suggested implementation.

@NoureldinYosri
Copy link
Collaborator Author

I'm willing to bet that my implementation is faster. the most expensive operation is not the qubit comparison but the call to commutes, caching the sorted qubits in a map is not optimal since the getting the sorted tuple should do at most a single comparison (the operations are supposed to be either single or two qubit operations).

@pavoljuhas
Copy link
Collaborator

pavoljuhas commented Oct 22, 2024

I'm willing to bet that my implementation is faster. the most expensive operation is not the qubit comparison but the call to commutes

Challenge accepted :-)
It actually turned out to be the comparison of Qid-s , even when sorted, that was taking up the most time.
The runtime got about twice faster after mapping qubits to integers; another optimization was to check for operations with disjoint qubits before calling protocols.commutes - see pavoljuhas@1ae465e.

Here is a benchmark code to be saved as bench_isort.ipy -

import cirq
moment = cirq.Moment(cirq.CZ(cirq.q(i + 1), cirq.q(i)) for i in reversed(range(0, 64, 2)))
circuit = cirq.Circuit(100 * [moment])
%timeit -n1 cirq.transformers.insertion_sort_transformer(circuit)

and here is the outcome for the PR head and the optimized version:

# current head of PR #6776
$ git checkout a8277a63bcdf69f20a0a81b182a2643220bb23ff
$ ipython bench_isort.ipy

14.1 s ± 93.1 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

# proposed change, https://github.com/pavoljuhas/Cirq/tree/amend-6776
$ git fetch https://github.com/pavoljuhas/Cirq.git amend-6776
$ git checkout 1ae465e118520e376de2820a1469a150459f7973
$ ipython bench_isort.ipy

5.59 s ± 72.5 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Update - the benchmark time is down to 0.61 s after pavoljuhas@619eb56

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.

Create a transformer that reorders operations if they commute
3 participants