18 mapping #41
18 mapping #41
Conversation
ucc/transpilers/ucc_transpiler.py
Outdated
| """ | ||
| Transpiles the given quantum `circuit` using either default Qiskit or UCC default compiler passes, as specified by the `mode`. | ||
|
|
||
| Parameters: | ||
| circuit (qiskit.QuantumCircuit): The Qiskit circuit to transpile. | ||
| mode (str): 'qiskit' or 'ucc', specifies which set of transpiler passes to use. | ||
| backend: Can be a Qiskit backend or coupling map, or a list of connections between qubits. If None, all-to-all connectivity is assumed. |
There was a problem hiding this comment.
The function parameter is named 'target' but the docstring calls it 'backend'. I would recommend switching to use 'backend' since it's more descriptive.
There was a problem hiding this comment.
'Backend' has a very specific meaning in Qiskit. 'target' seems to be used malleably. Collectively, it's a bit confusing. We can use backend as long as it's clear it's not a qiskit backend or pick a new term altogether. Let's agree on one and I can change it.
There was a problem hiding this comment.
We can do something like target_device.
| """ | ||
| Perform spectral graph matching between two graphs of unequal sizes. | ||
| Parameters: | ||
| adj1 (np.array): Adjacency matrix or Laplacian of graph 1 |
There was a problem hiding this comment.
The parameters listed here do not match the argument names.
There was a problem hiding this comment.
I know this may seem like a nitpick, but when parameters listed in the docstring don't match the code, I can't tell if they are referring to the same thing or if they are a leftover artifact from a previous implementation.
| Returns: | ||
| np.array: Index mapping from graph 1 to graph 2 | ||
| """ | ||
| #Ensure that the adjacency matrices are of the same size |
There was a problem hiding this comment.
The docstring states it performs "spectral graph matching between two graphs of unequal sizes" but the first thing it does is throw an error if the graphs are not the same shape? Which is the intended behavior?
There was a problem hiding this comment.
I'll fix the docstrings.
There was a problem hiding this comment.
Do you want the fixed docstrings in this branch or a different branch?
| coupling_map = CouplingMap(couplinglist=coupling_list) | ||
| self.pass_manager.append(SpectralMapping(coupling_list)) | ||
| self.pass_manager.append(SabreLayout(coupling_map=coupling_map)) | ||
| self.add_local_passes(1) |
There was a problem hiding this comment.
Why is self.add_local_passes() called within self.mapping_passes and not self.run()? The local passes are not really mapping passes are they?
There was a problem hiding this comment.
Since you need to do run the local optimizer over the whole circuit again after mapping since the latter adds new gates.
There was a problem hiding this comment.
Huh, but would the local_passes() then potentially cancel out some gates that you want in place for the qubit routing?
There was a problem hiding this comment.
That's a good consideration, but that doesn't happen since removing the routing SWAPs would require a global analysis of the circuit. It's possible that for very small circuits this could be an issue and that should be checked.
| def add_map_passes(self, coupling_list = None): | ||
| if coupling_list is not None: | ||
| coupling_map = CouplingMap(couplinglist=coupling_list) | ||
| self.pass_manager.append(SpectralMapping(coupling_list)) |
There was a problem hiding this comment.
This isn't a blocker, but I think you mentioned the transpiler seems to be ignoring the initial SpectralMapping choice of connected subgraph. Do if you have a sense of why that is? For instance, is it related to the coupling map or something about sabre_swap?
There was a problem hiding this comment.
From https://docs.quantum.ibm.com/api/qiskit/qiskit.transpiler.passes.SabreLayout, the SabreLayout pass takes any layouts you pass to it as a suggestion, but still does trials with random layouts. If you have a sufficiently sparse connectivity such as the ones with the Qiskit backends that we are using to benchmark, all layouts are approximately the same, and so likely there won't be much difference from random layout. With denser layouts, there should be a difference, but this needs to be tested more thoroughly.
"In addition to starting with a random initial Layout the pass can also take in an additional list of starting layouts which will be used for additional trials. If the sabre_starting_layouts is present in the property set when this pass is run, that will be used for additional trials. There will still be layout_trials of full random starting layouts run and the contents of sabre_starting_layouts will be run in addition to those. The output which results in the lowest amount of swap gates (whether from the random trials or the property set starting point) will be used. The value for this property set field should be a list of Layout objects representing the starting layouts to use. If a virtual qubit is missing from an Layout object in the list a random qubit will be selected.
No description provided.