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

Use rustworkx version of token_swapper #10001

Merged
merged 14 commits into from
Jul 13, 2023
Merged

Conversation

enavarro51
Copy link
Contributor

Summary

Call the rustworkx version of token_swapper

Details and comments

This PR should be put on hold until release of rustworkx version 0.13.0.

This replaces the python version of transpiler/passes/routing/algorithms/token_swapper with a call to the rustworkx version.

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Apr 20, 2023
@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:

@mtreinish mtreinish added the on hold Can not fix yet label Apr 20, 2023
@mtreinish
Copy link
Member

rustworkx 0.13.0 was released yesterday so we can move forward with this now

@coveralls
Copy link

coveralls commented Jun 9, 2023

Pull Request Test Coverage Report for Build 5547501913

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 8 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.01%) to 86.019%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
qiskit/transpiler/passes/routing/algorithms/token_swapper.py 1 92.31%
qiskit/transpiler/passes/routing/layout_transformation.py 1 76.09%
crates/qasm2/src/lex.rs 5 90.89%
Totals Coverage Status
Change from base Build 5547283790: -0.01%
Covered Lines: 71844
Relevant Lines: 83521

💛 - Coveralls

@enavarro51 enavarro51 marked this pull request as ready for review June 9, 2023 15:16
@enavarro51 enavarro51 requested review from a team, woodsp-ibm and ElePT as code owners June 9, 2023 15:16
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @ElePT
  • @Qiskit/terra-core
  • @woodsp-ibm

@mtreinish mtreinish added performance and removed on hold Can not fix yet labels Jul 12, 2023
@mtreinish mtreinish added this to the 0.25.0 milestone Jul 12, 2023
@mtreinish mtreinish self-assigned this Jul 12, 2023
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.

Overall this looks great! Thanks for doing this and also implementing the rustworkx side, this will have real performance improvements for the control flow path and also enables us to support control flow in sabre (see #10366 ). I left a small inline comment around the rng usage. The other thing is we need to bump the minimum version for rustworkx in the requirements.txt file, because with this change terra needs rustworkx 0.13.0. We also should have a matching upgrade release note to document that minimum requirements change.

todo_nodes.remove(node)
# Since integer seed is used in rustworkx, take random integer from np.random.randint
# and use that for the seed.
seed = np.random.randint(1, 10000, size=1)[0]
Copy link
Member

Choose a reason for hiding this comment

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

I think we probably should do:

Suggested change
seed = np.random.randint(1, 10000, size=1)[0]
seed = self.seed.integers(1, 10000)

so that we're respecting the input seed to the class.

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 thing. Done in 39617ec.

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, thanks for the quick update. I'll try to get 0.13.1 out before the final release so we can unskip the test too.

@mtreinish mtreinish enabled auto-merge July 13, 2023 20:27
@mtreinish mtreinish added the Changelog: API Change Include in the "Changed" section of the changelog label Jul 13, 2023
@mtreinish mtreinish added this pull request to the merge queue Jul 13, 2023
Merged via the queue into Qiskit:main with commit eb7530e Jul 13, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo performance
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants