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 linear synthesis algorithm for LNN #9098

Merged
merged 18 commits into from
Nov 29, 2022

Conversation

ShellyGarion
Copy link
Member

Summary

Adding a synthesis algorithm for linear reversible circuits (containing only CX gates) for linear-nearest-neighbor connectivity.
See #9036

Co-authored by Ben Zindorf.

Details and comments

According to the paper: https://arxiv.org/abs/quant-ph/0701194
The depth of an n-qubit linear reversible circuit is bounded by 5n.

@ShellyGarion ShellyGarion requested a review from a team as a code owner November 8, 2022 08:35
@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:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Nov 8, 2022

Pull Request Test Coverage Report for Build 3575999269

  • 123 of 125 (98.4%) changed or added relevant lines in 4 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.06%) to 84.599%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/synthesis/linear/linear_depth_lnn.py 117 119 98.32%
Files with Coverage Reduction New Missed Lines %
src/sabre_swap/layer.rs 2 98.95%
qiskit/pulse/library/waveform.py 3 91.49%
Totals Coverage Status
Change from base Build 3565432800: 0.06%
Covered Lines: 62983
Relevant Lines: 74449

💛 - Coveralls

@ShellyGarion ShellyGarion changed the title [WIP] Adding linear synthesis algorithm for LNN Adding linear synthesis algorithm for LNN Nov 8, 2022
@alexanderivrii alexanderivrii added this to the 0.23.0 milestone Nov 9, 2022
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks @ShellyGarion! Overall, this looks good to (and I have the benefit of knowing that this code works correctly in practice). I have left a few minor comments, nothing too deep.

qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the improvements! I have left a few more requests for minor changes, otherwise this looks very good.

My main problem when looking at this code (which has nothing to do with the code quality itself) it that the proof for lemma 7.3 in the paper is quite involved and does not fully describe how to implement it efficiently. I now see that Ben came up with the idea based on using the inverse matrix to get an efficient implementation.

Other than that the code follows the paper quite closely, and contains very helpful comments.

qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
alexanderivrii
alexanderivrii previously approved these changes Nov 10, 2022
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks @ShellyGarion, LGTM!

return cx_instructions_rows_m2nw, cx_instructions_rows_nw2id


def synth_cnot_depth_line_kms(mat):
Copy link
Member

Choose a reason for hiding this comment

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

Is this including in the docs tree anywhere? We should make sure that the new function we're advertising in the release notes has its documentation published.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not sure if we want to have this standalone function in the API docs, or allow to access it only via the HLS synthesis plugin

Copy link
Member Author

Choose a reason for hiding this comment

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

But if we add this algorithm to docs, then we should also add Patel-Markov-Hayes as well

qiskit/synthesis/linear/linear_depth_lnn.py Outdated Show resolved Hide resolved
qiskit/synthesis/linear/linear_depth_lnn.py Show resolved Hide resolved
alexanderivrii
alexanderivrii previously approved these changes Nov 15, 2022
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks, @ShellyGarion, looks good to me (including downloading documentation artifacts to see how docs look like).

alexanderivrii
alexanderivrii previously approved these changes Nov 16, 2022
@ShellyGarion ShellyGarion added automerge Changelog: New Feature Include in the "Added" section of the changelog labels Nov 29, 2022
@mergify mergify bot merged commit 61e0baf into Qiskit:main Nov 29, 2022
Cryoris pushed a commit to Cryoris/qiskit-terra that referenced this pull request Jan 12, 2023
* update init file

* add a test for kms synthesis for lnn

* add a synthesis algorithm based on kms for lnn in depth 5n.

Co-authored-by: Ben Zindorf <benzindorf@gmail.com>

* minor fixes in linear_depth_lnn

* fix import

* add release notes

* updates following review

* more updates following review

* add synth_cnot_depth_line_kms to docs

* updates following review comments

* fix init

* add Patel-Markov-Hayes to docs

* update docstring

* add Patel-Markov-Hayes to release notes

Co-authored-by: Ben Zindorf <benzindorf@gmail.com>
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