-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Make VF2Layout pass Target aware #7735
Conversation
This commit updates the VF2Layout pass to be target aware. It adds a new kwarg for specifying a target and if it's specified that target object is used to define the parameters for finding the isomorphic mapping used for the layout. However, with this commit the extra information contained in the target isn't really leveraged yet and just the global coupling graph and measurement error rates are pulled from the target just as in the BackendV1 case. This commit is mostly to facilitate future expansion where we will improve the layout scoring heuristic used and having the full set of data available in the target will be useful. Part of Qiskit#7113
|
I left the layout scoring algorithm the same for the target path here. I figured we can update this as part of #7722 and factor in all instruction errors when we score a layout. I also added |
Pull Request Test Coverage Report for Build 2068231268
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff aside, this looks good to me. I've left the review since Luciano is assigned but away from work.
for bit in bits: | ||
score += self.coupling_map.graph.out_degree( | ||
bit | ||
) + self.coupling_map.graph.in_degree(bit) | ||
return score |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pre-existing and doesn't need to be changed (nor should it, this close to release!), but it's weird to me that this path doesn't divide by the size of the graph, unlike other paths this approximation is used in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly a matter of relative values on the other qubits for the heuristic score that's returned. In the other paths when we have an error rate on some qubits that's always going to be < 1. So in the other paths we divide by the number of nodes in the absence of an error rate to make the value < 1 to hopefully not score super unfavorably towards qubits that could be potentially good and just happen to be missing an error rate in the backend but are still defined in the connectivity graph.
In the case of this path when we have no error rates it doesn't really matter because all the other scores are going to be in the same situation so we want to prefer to use a node with lower degree. We could divide but we don't have to as it wouldn't change the relative score compared to the other qubits.
Co-authored-by: Jake Lishman <jake@binhbar.com>
if self.target is not None: | ||
if "measure" in self.target: | ||
for bit in bits: | ||
props = self.target["measure"].get((bit,)) | ||
if props is None or props.error is None: | ||
score += ( | ||
self.coupling_map.graph.out_degree(bit) | ||
+ self.coupling_map.graph.in_degree(bit) | ||
) / len(self.coupling_map.graph) | ||
else: | ||
score += props.error | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice this before because of the nesting - if self.target is not None
but measure not in self.target
, we just return a score of 0 for all layouts now. I mean, I can't imagine a situation where measure
isn't in the target, but we may need to fall back to the "dumb" calculation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah ideally I would have just fixed the heuristic to be more general like I did in #7776 for dense layout, but I didn't want to step on @1ucian0's toes as he had started working on a better heurstic in #7722. I'm generally not a fan of doing this with a hardcoded measurement lookup by name like this. I can update the logic really quickly to fix this gap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in: f7fb8a5
This commit fixes an edge case in the heurstic scoring where if a target is present but doesn't have measurement defined we always return a score of 0. In the case measure ment doesn't have measurement defined this will fall back to looking at the degree of the qubit instead of trying to use the readout error rate.
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking. Some comments here and there.
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Summary
This commit updates the VF2Layout pass to be target aware. It adds a new
kwarg for specifying a target and if it's specified that target object
is used to define the parameters for finding the isomorphic mapping used
for the layout. However, with this commit the extra information contained in
the target isn't really leveraged yet and just the global coupling graph
and measurement error rates are pulled from the target just as in the
BackendV1 case. This commit is mostly to facilitate future expansion where
we will improve the layout scoring heuristic used and having the full set
of data available in the target will be useful.
Details and comments
Part of #7113