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

Adding permutation synthesis algorithm for LNN #9082

Merged
merged 21 commits into from
Nov 17, 2022

Conversation

alexanderivrii
Copy link
Contributor

@alexanderivrii alexanderivrii commented Nov 6, 2022

Summary

This commit add the depth-efficient synthesis algorithm for Permutation over the linear nearest-neighbor architecture (LNN) following the paper https://arxiv.org/abs/quant-ph/0701194, chapter 6. This is the second task in #9036.

Any permutation can be synthesized over LNN using a network of SWAP gates with depth at most n and size at most n(n-1)/2 (where both depth and size are measured with respect to SWAPs), or equivalently with depth at most 3n and size at most 3n(n-1)/2 (measured with respect to CNOTs).

Additional (minor) changes include improving the documentation of what a "permutation pattern" is, and moving _get_ordered_swaps to qiskit/synthesis/permutation/permutation_utils.py (and test_get_ordered_swap to test/python/circuit/library/test_permutation.py.

The next step (that would be handled in a follow-up PR) is to add a high-level synthesis plugin for Permutation (this may also require changing Permutation from QuantumCircuit to Gate).

Co-authored with https://github.com/NirGavrielov (Nir Gavrielov).

@coveralls
Copy link

coveralls commented Nov 6, 2022

Pull Request Test Coverage Report for Build 3490868680

  • 37 of 37 (100.0%) changed or added relevant lines in 5 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 84.614%

Files with Coverage Reduction New Missed Lines %
src/sabre_swap/layer.rs 2 98.95%
Totals Coverage Status
Change from base Build 3490867390: 0.04%
Covered Lines: 62690
Relevant Lines: 74089

💛 - Coveralls

@alexanderivrii alexanderivrii added the Changelog: New Feature Include in the "Added" section of the changelog label Nov 6, 2022
@alexanderivrii alexanderivrii added this to the 0.23.0 milestone Nov 6, 2022
@alexanderivrii alexanderivrii requested a review from a team as a code owner November 6, 2022 09:24
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

…vity and has depth guaranteed by the algorithm
Co-authored-by: Nir Gavrielov <[email protected]>
ShellyGarion
ShellyGarion previously approved these changes Nov 9, 2022
Copy link
Member

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mtreinish
Copy link
Member

Oh, also I realized I think we're not adding this new function to the doc tree anywhere we probably should add it so we're publishing the documentation for this algorithm.

@alexanderivrii
Copy link
Contributor Author

@mtreinish, thanks for the comments, it took me quite a number of attempts to get the docstrings working, but now all seems to be good.

@ShellyGarion ShellyGarion self-requested a review November 16, 2022 09:12
ShellyGarion
ShellyGarion previously approved these changes Nov 16, 2022
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Thanks for making the updates, in my previous review. Looking through the code again I caught one potential inefficiency which should be easy to fix. Other than that I think this probably good to go.

swap_list = []
for i, val in enumerate(permutation):
if val != i:
j = permutation.index(i)
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid using the index() call here, it basically is an O(n) lookup on each iteration making this loop scale quadratically. If you need to get the index of i from permutation, I would build a map outside the loop that can do this look up in constant time. That way this function is O(2n) instead of O(n**2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point. For the record, this function existed before, I have moved it to permutation_utils.py without any modifications. But I agree that it would be nice to make it more efficient. Hmm, it might be just a tiny bit trickier than what you have suggested, as the permutation (and hence also index()) changes throughout the loop, but it's clear that a linear algorithm should be possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And that was very easy, done in 15f1228.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the quick update.

@mergify mergify bot merged commit 02788a1 into Qiskit:main Nov 17, 2022
@alexanderivrii alexanderivrii deleted the permutations-over-lnn branch November 20, 2022 08:44
Cryoris pushed a commit to Cryoris/qiskit-terra that referenced this pull request Jan 12, 2023
* Adding permutation synthesis algorithm for LNN

* release notes

* Checking that the synthesized permutation adheres to the LNN connectivity and has depth guaranteed by the algorithm

* Adding tests for 15 qubits

Co-authored-by: Nir Gavrielov <[email protected]>

* fixing assert

* improving description message for _get_ordered_swap

* applying suggestions from code review

* minor

* attempt to fix docstring

* Another attempt to fix docsting

* another attempt to fix docstring

* temporarily simplifying docstring to see if this passes docs build

* adding blank line

* another attempt

* Restoring docstring

* removing extra line

* speeding up _get_ordered_swap based on review comments

Co-authored-by: Nir Gavrielov <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog synthesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants