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

Follow-up on routing commuting 2q gates #7979

Merged
merged 23 commits into from
May 11, 2022
Merged

Follow-up on routing commuting 2q gates #7979

merged 23 commits into from
May 11, 2022

Conversation

eggerdj
Copy link
Contributor

@eggerdj eggerdj commented Apr 22, 2022

Summary

This is a PR to address the remaining comments in #7813 .

Details and comments

See the response to the comments in #7813.

@eggerdj eggerdj requested a review from a team as a code owner April 22, 2022 16:17
@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 Apr 22, 2022

Pull Request Test Coverage Report for Build 2302788972

  • 9 of 9 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 84.376%

Totals Coverage Status
Change from base Build 2302148037: 0.01%
Covered Lines: 54497
Relevant Lines: 64588

💛 - Coveralls

@eggerdj eggerdj changed the title [WIP] Follow-up on routing commuting 2q gates Follow-up on routing commuting 2q gates May 4, 2022
Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

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

looks good, thanks for doing this follow up

from .swap_strategies.commuting_2q_gate_router import Commuting2qGateRouter
from .swap_strategies.swap_strategy import SwapStrategy
from .commuting_2q_gate_routing.commuting_2q_gate_router import Commuting2qGateRouter
from .commuting_2q_gate_routing.swap_strategy import SwapStrategy
Copy link
Member

@ajavadia ajavadia May 10, 2022

Choose a reason for hiding this comment

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

can you remove SwapStrategy from init of qiskit.transpiler.passes? It's not a pass (contrary to all the other things in that namespace, which so far has been a nice place to show all passes in qiskit). I think people can import it from either qiskit.transpiler.passes.routing.swap_strategy or at least qiskit.transpiler.passes.routing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this should cover it: c4d2f20

@mergify mergify bot merged commit c8ad6cf into Qiskit:main May 11, 2022
@eggerdj eggerdj deleted the routing_commuting_gates_follow_up branch May 11, 2022 06:02
ajavadia pushed a commit to ajavadia/qiskit that referenced this pull request May 31, 2022
* * Renamed directory.

* * Typo.

* * Commuting2qBlocks name

* * logical -> virtual.

* * from_line in the example.

* * Reno name fix.

* * Reno rewording.

* * removed unnecessary line in example.

* * Black.

* * Started designing test on non-line graph.

* * Added test on a T device.

* * Renamed Communting2qBlocks to Commuting2qBlock

* * Made test robust to arbitraryness of commuting gate order.
* Added test.

* * Added test and black.

* * Fix missing gate test exception refactor.

* * Bug fix with test.

* * Removed SwapStrategy from the passes init.

* * black
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants