Improve performance of VF2 scoring and add support for scoring passes

[mtreinish]

Summary

This commit makes 2 key changes to the vf2 layout pass. The first is it migrates the scoring routine to rust. When running vf2 layout and vf2 post layout we're bottle necked by the performance of the scoring of a layout since in practice scoring a large circuit ends up taking more time than the vf2_mapping() function. To address this the scoring function is migrated to rust where the iteration will be much faster. To enable this rust migration the average error map is made into an ErrorMap class which can be efficiently be accessed by reference from rust. This additionally also enables a convenient interface for future expansion of the vf2 layout passes. The VF2LayoutPass and VF2PostLayout passes will now both look for a "vf2_avg_error_map" entry in the property set which contains a ErrorMap used for scoring. If present that array will be used for scoring instead of the computing one from the target's error rates. This will enable custom analysis passes to be run pre-layout to compute or inject a custom scoring heuristic.

Details and comments

TODO:

• Fix test failures. Fails with faulty qubits (I assume the backend properties mapping is broken and only worked by chance before this) and fake melbourne (which had a weird broken qubit and I bet the stored payload is broken)
• Benchmark and potentially tune performance

[qiskit-bot]

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
• @kevinhartman
• @mtreinish

[coveralls]

Pull Request Test Coverage Report for Build 3500758612

• 138 of 164 (84.15%) changed or added relevant lines in 7 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.04%) to 84.575%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/layout/vf2_utils.py	39	41	95.12%
src/vf2_layout.rs	54	57	94.74%
src/error_map.rs	18	39	46.15%

Totals
Change from base Build 3500114179:	-0.04%
Covered Lines:	62629
Relevant Lines:	74051

---

💛 - Coveralls

[jakelishman]

Moving scoring to Rust generally seems sensible to me. Most of the comments below are very minor.

My two main things are:

• I'm concerned we're overloading NaN in the gate-error matrix with two incompatible meanings.
• I think that even for 1000q systems, the 2D matrix is probably going to be fine, but there's a possibility we might end up with better cache locality in the scoring for large systems if we considered some sparser structure, since we generally expect that most real-world systems will have limited connectivity, so this 2D matrix will in practice be very sparse. This certainly doesn't need investigating for this PR, just wondering if you'd thought anything about it?

[jakelishman]

This flag seems unnecessary in Rust with things now represented as an error_matrix? I'd have thought that it's just built in during the construction of the matrix.

[mtreinish]

It's not necessarily reflected in the error matrix, it really depends on the backend properties since we could end up with an error rate defined directionally in the backend and it's hard to know until after we finish building the matrix so we don't build the error matrix bidirectional if strict_direction=False. It becomes more a question of scoring behavior than representing it. If strict_direction = False the and err_mat[[0, 1]] is NaN it will try err_mat[[1, 0]]

[jakelishman]

Yeah, that's fair enough then. In all the situations I could think of, the backend would just have constructed the directional error matrix itself, but if there's a chance those are decoupled, then it's right to include the swap.

That said, it feels a bit odd in the scoring that we don't take into account both sides of the link in other cases. If it's not strict directionality, but the two directions have different average errors, it feels weird that we don't take that into account somehow? In my mental model, that situation shouldn't be possible, but if the error matrix and strict directionality aren't coupled at the level of the backend, it starts to feel possible and unclear in how it should be handled.

[mtreinish]

It's a tradeoff to find a path vs not finding a path. There are two modes of operation right now with the vf2 mapping, either we work with directed graphs and respect the direction of gates on the backend or we treat every edge as weak and work with undirected edges and then rely on GateDirection later to correct things. In the former case if the edge is defined bidirectionally with different error rates rustworkx will return a different mapping with either direction and score them differently. But for the latter case we can't really make an assertion about the order of the qubits for the qargs because it's not explicitly set.

That being said I think you're right for the undirected case maybe we should be looking at the other direction in scoring if both directions have defined error rates. We'll have to think of the best way to do this. (for this I just copied the scoring algorithm we were using before anyway)

[mtreinish]

I've updated the PR to use a custom hash map based class instead of a 2d numpy array to represent the average error map. This should both be much more memory efficient and also make the usage a bit clearer for people to interact with.

[mtreinish]

To show the performance improvements I threw together a small test script:

import statistics
import time

from qiskit.transpiler.passes.layout import VF2Layout
from qiskit.circuit import QuantumCircuit
from qiskit.providers.fake_provider import FakeMumbaiV2
from qiskit.converters import circuit_to_dag

qc = QuantumCircuit(7)

qc.h(0)
qc.cz(0, 1)
qc.cz(1, 2)
qc.cz(2, 3)
qc.measure_all()
dag = circuit_to_dag(qc)
backend = FakeMumbaiV2()

times = []
vf2_pass = VF2Layout(target=backend.target, max_trials=-1)
for i in range(5):
    print(f"Run {i}")
    start = time.perf_counter()
    vf2_pass.run(dag)
    stop = time.perf_counter()
    times.append(stop - start)
    print(stop - start)
print(statistics.geometric_mean(times))

This script is a worst case from a scoring perspective, it's mapping a line with 4 nodes and 3 free nodes onto the coupling graph. This means there are a lot of possible permutations for valid isomorphic mappings but each is trivial for rustworkx to calculate. It also turns off any limits in the pass so it will fully iterate through all available mappings.

Running this script with this PR applied returned:

6.825820831970144

Then running it on main:

14.780817176541099

This obviously is a best case improvement. In more realistic testing with limits set I was seeing a ~10% improvement over main with this PR applied.

[kevinhartman]

LGTM.

Should we add some test to make sure the avg_error_map property set setting is properly loaded/used when present?

[mtreinish]

LGTM.

Should we add some test to make sure the avg_error_map property set setting is properly loaded/used when present?

Good call, I added a test with avg_error_map in: 50aab71

[kevinhartman]

LGTM!