Filter/ignore qubits in Target without any operations

[mtreinish]

Summary

Building off #9840 which adds full path support in all the preset pass
managers for targeting backends with a disconnected coupling graph,
this commit adds support for ignoring qubits that do not support any
operations. When a Target is generated from #9911 with filter_faulty
set to True this will potentially result in qubits being present in
the Target without any supported operations. In these cases the
layout passes in the transpiler might inadvertently use these qubits
only to fail in the basis translator because there are no instructions
available. This commit adds filtering of connected components from the
list of output connected components if the Target does have any
supported instructions on a qubit.

This works by building a copy of the coupling map's internal graph
that removes the nodes which do not have any supported operations.
Then when we compute the connected components of this graph it will
exclude any components of isolated qubits without any operations
supported. A similar change is made to the coupling graph we pass to
rustworkx.vf2_mapping() inside the vf2 layout family of passes.

Details and comments

This PR is based on #9840 (and in turn #9802 which is the parent of #9840). Once #9840 is merged I'll rebase this
PR to isolate just the local changes here. In the meantime you can refer to:

mtreinish/qiskit-core@full-transpile-disjoint-support...mtreinish:qiskit-core:filter-no-op-standalone-qubits

To see just the contents of this PR.

[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

[coveralls]

Pull Request Test Coverage Report for Build 4736853646

• 49 of 50 (98.0%) changed or added relevant lines in 11 files are covered.
• 11 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-0.01%) to 85.858%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/routing/bip_mapping.py	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
qiskit/circuit/tools/pi_check.py	1	91.23%
qiskit/transpiler/passes/layout/dense_layout.py	1	96.08%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	95.37%
qiskit/pulse/library/waveform.py	3	91.67%
crates/qasm2/src/lex.rs	5	90.63%

Totals
Change from base Build 4735488897:	-0.01%
Covered Lines:	70715
Relevant Lines:	82363

---

💛 - Coveralls

[jakelishman]

I'm a little concerned by the number of transpiler passes that needed to be touched to make this work because they can't call the new logic in CouplingMap for their use-cases. It makes it feel like the API we're putting in here isn't entirely correct, and the number of changes makes it feel like it's going to be easy for bugs to slip in in the future where people forget to maintain this property.

As a second point: ought we to update all the other CouplingMap methods as well, if we're going with this API?

[jakelishman]

This feels like a bit of a weird interface considering how we used things. We first create a CouplingMap from a Target, but then we have to pass the target to connected_components again when calling methods on the derived map? It feels like something we perhaps ought to be encoding at construction time, maybe, or alternatively having connected_components to be an associated function that can be called either as a method or directly as a function passing a target in the first case?

Neither of those feel too great to me, though.

[mtreinish]

Yeah, I wasn't happy with this interface really either. My first thought was that the target generated coupling map actually currently annotates the nodes and edges with some of the necessary information. But managing that was tricky and also the information wasn't exactly easy to use that way. I like your suggestion of using an associated method I can adjust the type signature on the disjoint functions to match what the passes are using to take in a target instead of a coupling map and then just generate it on the fly. It's not perfect but it at least avoids the back and forth between the objects.

[mtreinish]

I reworked the filtering to be a function of the Target class and then updated the disjoint utils methods to build the coupling map connected components from either a target or a coupling map in: c6a1e89 I think this keeps the API separation a bit more clear as it's the target which owns all the aspects of which operations are supported on which qubits.

[jakelishman]

This seems a bit confusing (we have both qargs and direct calls to target.qargs) and inefficient - we scan through all qargs in the target for each of the node indices in the happy path (no faulty gates), which feels a bit quadratic. Does something like

has_operations = set(itertools.chain.from_iterable(target.qargs))
for index in graph.node_indices():
    if index not in has_operations:
       graph.remove_node(index)

I'm not 100% certain that that's the same logic, though.

[jlapeyre]

I'm confused about the purpose of qargs = target.qargsin line 481. The loop variable qargs in line 484 is not introduced in a new scope, so it overwrites the previous definition in case graph.node_indices() is not empty. And otherwise, it is never used.
EDIT: in fact this tox -epy311 -- -n test.python.compiler.test_transpiler passes with line 481 removed.

[mtreinish]

I reworked the logic using your suggestion as part of doing the filtering in the target as part of: c6a1e89

[mtreinish]

I'm a little concerned by the number of transpiler passes that needed to be touched to make this work because they can't call the new logic in CouplingMap for their use-cases. It makes it feel like the API we're putting in here isn't entirely correct, and the number of changes makes it feel like it's going to be easy for bugs to slip in in the future where people forget to maintain this property.

As a second point: ought we to update all the other CouplingMap methods as well, if we're going with this API?

Well for the most part these changes were just giving passing the target to the disjoint utils function because all of those functions were written using the coupling map directly and didn't have the necessary full context from the target to filter the components out of the coupling map (vf2* needs to be handled manually because it operates on a lower level, the raw graph object, than they other layout passes). But, I think you're inline comment about how we're passing the target is making me think we need to do the filtering directly on the target itself either as part of build_coupling_map() with a flag or something. I'll play with it and see if I can come with a better separation of APIs.

[jlapeyre]

You might want to update the method name in the docstring here
https://github.com/Qiskit/qiskit-terra/blob/4d4d292c8391f9400a7a5607e6d03f98a366d387/qiskit/transpiler/coupling.py#L461

as part of this PR instead of relegating to another issue/PR. (This link is to the line in main. Last I checked, you still can't refer to a line not touched by the PR.)

[mtreinish]

You might want to update the method name in the docstring here

https://github.com/Qiskit/qiskit-terra/blob/4d4d292c8391f9400a7a5607e6d03f98a366d387/qiskit/transpiler/coupling.py#L461

as part of this PR instead of relegating to another issue/PR. (This link is to the line in main. Last I checked, you still can't refer to a line not touched by the PR.)

This PR is no longer touching the coupling.py module after the most recent update. Do you want to just push up a quick PR to update the doc example since you caught the issue?

[jlapeyre]

Do you want to just push up a quick PR to update the doc example since you caught the issue

Yes.

I didn't scroll down to see that you no longer touch coupling.py.

[kevinhartman]

LGTM!

[jakelishman]

This interface looks much cleaner to me as well, thanks for the changes on that.