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

better unrolling in preset passmanagers #6133

Merged
merged 10 commits into from
Apr 9, 2021

Conversation

ajavadia
Copy link
Member

@ajavadia ajavadia commented Apr 1, 2021

#5609 introduced a performance regression in time and quality of transpilation by doing unrolling at the end of transpilation level0, 1, 3.
This reclaims some of the performance back by doing the unrolling in an optimization loop. But the unrolling is necessary because the generated circuits were actually wrong before (they generated circuits that did not respect the basis). I added a release note to highlight this fact, and added tests (these tests would have failed prior to #5609 )

This also points to a need for better testing in the randomized transpiler passes to make sure the generated circuit is equivalent and respects the constraints in transpile().

@ajavadia ajavadia requested a review from a team as a code owner April 1, 2021 14:05
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, it would be good to include this in 0.17.0, but given the timing I think we just missed it. We'll have to include it in a 0.17.1 in a week or two. Just one small comment on the release note.

@mtreinish mtreinish added Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable labels Apr 1, 2021
@mtreinish
Copy link
Member

Before I merge this, I did have one question. Do we know which pass the gates outside of the basis were coming from? It doesn't really effect this PR because I think having the _unroll stage run on each iteration of the optimization loop makes sense, but I'm just curious if there is a pass we could/should make basis aware.

@mtreinish
Copy link
Member

So I dug into this a bit, and the pass that was introducing U2s for level 0 and level 1 before was CXDirection which @ajavadia already removed to the preset pass managers and moved us to the GateDirection pass. However GateDirection is just now using H gates which are not guaranteed to be in the basis either so we still need to unroll to basis to ensure the output from GateDirection is converted to the specified basis set. I do think this raises bigger questions about how we track attributes of transformation passes in the passmanager. Unrolling in the optimization loop works fine, but I wonder if we want attributes like preserves_basis, approximate, etc as a top level attributes of a pass. I think we kind of hacked it in with the approximation_degree kwarg recently for approximate passes, but having a more formal interface for these kinds of attributes might be useful (I'll open an issue about this, or just reuse #5978 and make this part of that interface).

@mergify mergify bot merged commit 9004023 into Qiskit:master Apr 9, 2021
mergify bot pushed a commit that referenced this pull request Apr 9, 2021
* move unroll inside optimization loop for level1-3

* add test and release note for bugfix of basis respect

* remove print

* Update releasenotes/notes/bugfix-respect-basis-d2204b9b7c403722.yaml

* Update releasenotes/notes/bugfix-respect-basis-d2204b9b7c403722.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 9004023)
mergify bot added a commit that referenced this pull request Apr 9, 2021
* move unroll inside optimization loop for level1-3

* add test and release note for bugfix of basis respect

* remove print

* Update releasenotes/notes/bugfix-respect-basis-d2204b9b7c403722.yaml

* Update releasenotes/notes/bugfix-respect-basis-d2204b9b7c403722.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 9004023)

Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants