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

Improve SabreSwap error message on unexpanded circuits #9909

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

jakelishman
Copy link
Member

Summary

SabreSwap requires that the input circuit is exactly the same size as the system implied by the coupling map, since this is used internally in Rust for the virtual-to-physical mapping. Failure to ensure this can lead to panics in the Rust code. This commit catches the failure case (for free) in Python space, and attempts to help the user work out what they needed.

Details and comments

This was prompted by a discussion in Slack about the confusing error, though I've done approximately the same thing myself before and got confused.

`SabreSwap` requires that the input circuit is exactly the same size as
the system implied by the coupling map, since this is used internally in
Rust for the virtual-to-physical mapping.  Failure to ensure this can
lead to panics in the Rust code.  This commit catches the failure case
(for free) in Python space, and attempts to help the user work out what
they needed.
@jakelishman jakelishman added Changelog: None Do not include in changelog mod: transpiler Issues and PRs related to Transpiler labels Apr 4, 2023
@jakelishman jakelishman added this to the 0.24.0 milestone Apr 4, 2023
@jakelishman jakelishman requested a review from a team as a code owner April 4, 2023 20:56
@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

Pull Request Test Coverage Report for Build 4612257839

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 85.407%

Totals Coverage Status
Change from base Build 4608901354: 0.003%
Covered Lines: 67398
Relevant Lines: 78914

💛 - Coveralls

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, this is straightforward enough. I was going to ask that we wait until after #9840 to ensure the conditions match in the case of disjoint coupling map. But I don't think it'll be an issue because layout should work and also at least in the preset pass managers we don't ever run this pass in practice. It does remind me that for #9840 after I rebase it I should probably add a release note for layout pass authors about the disconnected coupling maps and how to handle them.

@mtreinish mtreinish added this pull request to the merge queue Apr 5, 2023
@jakelishman
Copy link
Member Author

I'd actually forgotten that we had #9840 on the go when I wrote this. Yeah, for sure we can revisit the error messages as part of that too.

Merged via the queue into Qiskit:main with commit 5bd188b Apr 5, 2023
@jakelishman jakelishman deleted the better-sabre-error branch April 6, 2023 02:24
giacomoRanieri pushed a commit to giacomoRanieri/qiskit-terra that referenced this pull request Apr 16, 2023
`SabreSwap` requires that the input circuit is exactly the same size as
the system implied by the coupling map, since this is used internally in
Rust for the virtual-to-physical mapping.  Failure to ensure this can
lead to panics in the Rust code.  This commit catches the failure case
(for free) in Python space, and attempts to help the user work out what
they needed.
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
`SabreSwap` requires that the input circuit is exactly the same size as
the system implied by the coupling map, since this is used internally in
Rust for the virtual-to-physical mapping.  Failure to ensure this can
lead to panics in the Rust code.  This commit catches the failure case
(for free) in Python space, and attempts to help the user work out what
they needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: transpiler Issues and PRs related to Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants