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 efficiency of vf2 pass search with free nodes #9148

Merged
merged 6 commits into from
Jan 18, 2023

Conversation

mtreinish
Copy link
Member

Summary

This commit improves the efficiency of the VF2Layout and VF2PostLayout pass when there are qubits that only contain single qubit operations. Previously these passes would model these qubits as free nodes in the interaction graph. This would cause an explosion in the number of possible isomorphic mappings because vf2_mapping() will return a potential subgraph for every permutation of free nodes on the coupling graph. This means we end up spending a potentially huge amount of time scoring these permutations when we can more effectively place these free nodes as part of a dedicated step after placing the subgraph. This is only true for strict_direction=False because for strict_direction=True with a Target we need to rely on the subgraph isomorphism to ensure we're fully evaluating subgraphs comply with local operation availablility.

When there are free nodes in an interaction graph this changes the interaction graph to strip those out of the graph before checking for isomorphic subgraphs. This greatly reduces the search space and should speed up that iterative scoring. After we've selected the best subgraph of nodes with 2q interactions before returning the final layout we evaluate the free nodes and pick an available qubit with lowest 1q error for each free node.

Details and comments

This will have merge conflicts with #9026, I'll rebase either or depending on which merges first.

This commit improves the efficiency of the VF2Layout and VF2PostLayout
pass when there are qubits that only contain single qubit operations.
Previously these passes would model these qubits as free nodes in the
interaction graph. This would cause an explosion in the number of
possible isomorphic mappings because vf2_mapping() will return a
potential subgraph for every permutation of free nodes on the coupling
graph. This means we end up spending a potentially huge amount of time
scoring these permutations when we can more effectively place these free
nodes as part of a dedicated step after placing the subgraph. This is
only true for strict_direction=False because for strict_direction=True
with a Target we need to rely on the subgraph isomorphism to ensure
we're fully evaluating subgraphs comply with local operation
availablility.

When there are free nodes in an interaction graph this changes the
interaction graph to strip those out of the graph before checking for
isomorphic subgraphs. This greatly reduces the search space and should
speed up that iterative scoring. After we've selected the best subgraph
of nodes with 2q interactions before returning the final layout we
evaluate the free nodes and pick an available qubit with lowest 1q
error for each free node.
@mtreinish mtreinish added performance Changelog: None Do not include in changelog mod: transpiler Issues and PRs related to Transpiler labels Nov 16, 2022
@mtreinish mtreinish requested a review from a team as a code owner November 16, 2022 22:38
@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 Nov 16, 2022

Pull Request Test Coverage Report for Build 3952363540

  • 41 of 50 (82.0%) changed or added relevant lines in 5 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 84.837%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/layout/vf2_utils.py 25 26 96.15%
src/error_map.rs 4 7 57.14%
src/vf2_layout.rs 8 13 61.54%
Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 91.67%
Totals Coverage Status
Change from base Build 3952362369: -0.04%
Covered Lines: 65932
Relevant Lines: 77716

💛 - Coveralls

@mtreinish mtreinish added this to the 0.23.0 milestone Jan 12, 2023
@mtreinish mtreinish linked an issue Jan 12, 2023 that may be closed by this pull request
@jakelishman jakelishman self-assigned this Jan 12, 2023
@mtreinish mtreinish added the Rust This PR or issue is related to Rust code in the repository label Jan 12, 2023
@nonhermitian
Copy link
Contributor

Since I saw this pop up again, it might be nice to pick the free qubits based on the error that is worst. Namely, the 1Q errors are much smaller on IBM hardware than the measurement errors. So the latter as a score would make more sense. But that is not true in general, so one might have to query the device for which is worse. As most sane use of the transpiler would collapse multiple 1Q gates down, basing things on the meas error makes a lot of sense in most cases.

@jakelishman
Copy link
Member

Paul: 1q are much smaller, but a given circuit is also likely to have many 1q gates per measurement, so even in the situation you're describing, it's not necessarily a done deal that measurement will dominate, I think? For efficiency reasons in scoring, I think (but could be wrong) that it's impractical for us to produce the completely accurate score that Target does make available, but as long as the calculation of the (qargs -> estimated error) map that we score off is done only once, I think there's plenty of scope for tweaking the heuristics that go into the "estimated error". It'd be best to keep that outside this PR, though - this doesn't change the actual scoring numbers, it just removes isolated 1q islands from the connectivity graph to be laid out, and assigns them in n lg n time based on the same "estimated error".

One way is we could potentially look at treating "measure" and "1q gate" separately in the scoring, although as you say, measures aren't necessarily much worse on non-superconducting hardware. Alternatively, we could weight the average error per qubit proportional to the number of each instruction applied in the whole circuit.

@nonhermitian
Copy link
Contributor

So, unless I am misunderstanding, this PR addresses individual qubits with nothing other than 1Q gates on them, and measurements (without the measurement I could care less that the qubit is there). The thing is that in 99% of cases, the transpiler is going to take your 1Q gates and collapse them to a U3 and spit it back out in terms of at most 2 sx gates (on IBM hardware). sx gate error is on the order of 1e-4, where as measurements are ~1e-2. So if one worried about errors, in most cases measurements are the thing to worry about in this case.

@jakelishman
Copy link
Member

Oh yeah, that is true - I wasn't actually thinking about the context of this PR requiring each of those qubits to be non-interacting. We do include the measurement in the weight (arithmetic mean of each of the 1q operations inc measure available on that qubit), though, and if the measurements dominate the gates as you expect, then the heuristic we use will largely decay to what you're suggesting.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Code-wise this seems fine to me, minor question about scope aside. Do you want to add a performance "feature" release note?

Comment on lines -101 to +116
for node_index in bit_map.values():
bit_list[node_index] = sum(im_graph[node_index].values())
if strict_direction:
for node_index in bit_map.values():
bit_list[node_index] = sum(im_graph[node_index].values())
Copy link
Member

Choose a reason for hiding this comment

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

This (and the bit in build_average_error_map) look like a slightly separate bugfix. Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, although it's semi related I think (it's been a while since I looked at this patch). The strict direction case doesn't use the common base between the passes in the same way (since vf2post has to do a lot more checking to ensure direcitonality constraints and target basis are preserved). Splitting out the 1q piece from things broke things IIRC (only vaguely remember) so we needed the if condition to branch the logic on that otherwise it failed.

@nonhermitian
Copy link
Contributor

@jakelishman

If your taking the product of the 1Q and meas errors then all good. It looks like I was just interpreting this statement to mean only the latter was evaluated:

we evaluate the free nodes and pick an available qubit with lowest 1q error for each free node

@mtreinish
Copy link
Member Author

Code-wise this seems fine to me, minor question about scope aside. Do you want to add a performance "feature" release note?

I didn't really feel it was necessary. I was treating this more of a small bugfix than a performance improvement TBH. But I can add one if you think it's necessary.

@mtreinish
Copy link
Member Author

If your taking the product of the 1Q and meas errors then all good. It looks like I was just interpreting this statement to mean only the latter was evaluated:

we evaluate the free nodes and pick an available qubit with lowest 1q error for each free node

Yeah this was me just being a bit too precise and not explaining the context, I meant the 1q terms in the average error map that gets built internally. As Jake said by default that's the arithmetic mean of all the 1q operations (including measurement errors) reported by the backend.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I'm fine without a release note too, no worries.

@mergify mergify bot merged commit d4a63d9 into Qiskit:main Jan 18, 2023
@mtreinish mtreinish deleted the vf2-free-node-optimization branch March 21, 2023 19:08
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 5, 2023
In Qiskit#9148 a bug was introduced into the vf2 scoring code. In that PR the
vf2 passes were changed to treat standalone qubits as a special case to
improve the algorithmic efficiency of the pass. However, that PR
broke the scoring algorithm so that it was no longer factoring in the 1q
component of the interaction graph when scoring a layout. This meant
that when scoring a potential layout only the 2q error rates were been
factored into the score and the pass was potentially selecting worse
performing qubits. This commit fixes this error and ensures the scoring
is looking at all the error rates.
kevinhartman pushed a commit that referenced this pull request May 8, 2023
* Fix interaction graph vf2 scoring to include 1q component

In #9148 a bug was introduced into the vf2 scoring code. In that PR the
vf2 passes were changed to treat standalone qubits as a special case to
improve the algorithmic efficiency of the pass. However, that PR
broke the scoring algorithm so that it was no longer factoring in the 1q
component of the interaction graph when scoring a layout. This meant
that when scoring a potential layout only the 2q error rates were been
factored into the score and the pass was potentially selecting worse
performing qubits. This commit fixes this error and ensures the scoring
is looking at all the error rates.

* Update qiskit/transpiler/passes/layout/vf2_utils.py
mergify bot pushed a commit that referenced this pull request May 8, 2023
* Fix interaction graph vf2 scoring to include 1q component

In #9148 a bug was introduced into the vf2 scoring code. In that PR the
vf2 passes were changed to treat standalone qubits as a special case to
improve the algorithmic efficiency of the pass. However, that PR
broke the scoring algorithm so that it was no longer factoring in the 1q
component of the interaction graph when scoring a layout. This meant
that when scoring a potential layout only the 2q error rates were been
factored into the score and the pass was potentially selecting worse
performing qubits. This commit fixes this error and ensures the scoring
is looking at all the error rates.

* Update qiskit/transpiler/passes/layout/vf2_utils.py

(cherry picked from commit 0e44c5e)
mtreinish added a commit that referenced this pull request May 8, 2023
…10086)

* Fix interaction graph vf2 scoring to include 1q component

In #9148 a bug was introduced into the vf2 scoring code. In that PR the
vf2 passes were changed to treat standalone qubits as a special case to
improve the algorithmic efficiency of the pass. However, that PR
broke the scoring algorithm so that it was no longer factoring in the 1q
component of the interaction graph when scoring a layout. This meant
that when scoring a potential layout only the 2q error rates were been
factored into the score and the pass was potentially selecting worse
performing qubits. This commit fixes this error and ensures the scoring
is looking at all the error rates.

* Update qiskit/transpiler/passes/layout/vf2_utils.py

(cherry picked from commit 0e44c5e)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
* Fix interaction graph vf2 scoring to include 1q component

In Qiskit#9148 a bug was introduced into the vf2 scoring code. In that PR the
vf2 passes were changed to treat standalone qubits as a special case to
improve the algorithmic efficiency of the pass. However, that PR
broke the scoring algorithm so that it was no longer factoring in the 1q
component of the interaction graph when scoring a layout. This meant
that when scoring a potential layout only the 2q error rates were been
factored into the score and the pass was potentially selecting worse
performing qubits. This commit fixes this error and ensures the scoring
is looking at all the error rates.

* Update qiskit/transpiler/passes/layout/vf2_utils.py
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 performance priority: medium Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long transpile time for simple circuits
5 participants