-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Unroll3qOrMore before layout allocation stage in the levels 0, 1, and 2 #7251
Merged
Merged
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
51aeda9
level 0
1ucian0 f357411
level 0
1ucian0 69f6737
level 1 and 2
1ucian0 87fbe9f
level 1 and 2
1ucian0 f35a36a
Merge branch 'main' of github.com:Qiskit/qiskit-terra into fix/7156
1ucian0 f3abe50
always unroll
1ucian0 f7e71d8
reduce diff
1ucian0 268413a
reno
1ucian0 9a0093e
conflict
1ucian0 6a7f8e9
Merge branch 'main' into fix/7156
1ucian0 acb81bb
_unroll3q without coupling map
1ucian0 6714b78
Merge branch 'main' of github.com:Qiskit/qiskit-terra into fix/7156
1ucian0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
fixes: | ||
- | | ||
Fixed `#7156 <https://github.com/Qiskit/qiskit-terra/issues/7156>`__ . | ||
Many layout methods ignore 3-or-more qubit gates resulting in expected layout allocation decisions. | ||
The pass :class:`qiskit.transpiler.passes.Unroll3qOrMore` is now being executed before the layout pass. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we haven't laid out the qubits yet, what does the coupling map mean for the plugin? We could omit it, but then it starts to feel like we'll not following the calling conventions we laid out in the definition of the synthesis plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a good point. If the circuit doesn't have a layout set this will be doing the wrong thing, because the synthesis pass assumes it's going to have the bits in physical order in the dag. I think we should set this to
None
if run prior to layout.None
is still a valid value here to indicate there is no target coupling map set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I forgot we still passed through explicit
None
on that one. Yeah, that seems like the best solution for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggested test to add about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit tricky to test for because it doesn't come up with the default/built-in unitary synthesis mechanism since for >=3q it uses isometry which isn't coupling map aware. For the default plugin this only comes up with <=2q unitaries. It's only for potential plugins like AQC or BQSKit where this would come up, since the synthesis pass sends bit indices to the plugin assuming their in physical order: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/passes/synthesis/unitary_synthesis.py#L272-L276 (ie post routing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could set up a
unittest.mock.MagicMock
wrapper around the default synthesis plugin to catch all calls, then run the transpiler pass and assert that the first call hasmin_qubits=3
andcoupling_map=None
, perhaps? There's some examples of this ~~sort of test intest_unitary_synthesis_plugin.py
, like here: https://github.com/Qiskit/qiskit-terra/blob/7e118e81344cb05aada5082e3cc71c1c239baaa5/test/python/transpiler/test_unitary_synthesis_plugin.py#L220-L235There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testing is a bit artificial. Is it worthy?