-
Notifications
You must be signed in to change notification settings - Fork 26
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
Restructuring of the heuristic system #410
Conversation
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.
Many thanks for another great contribution. What a nice way to start the year!
I really like the direction this PR is taking. Makes the code much more organized.
Most of the comments below are really just comments. Feel free to read them, think about them, and discard them if you don't agree with them.
I think the only point where I might not be that happy is the Architecture
class as some redundancy seems to be introduced by the PR. We should be able to clarify this quickly though!
Alright, first of all thank you very much for the quick review! Some of the design choices in this PR were made with future plans/issues in mind, which are (or were previously) not mentioned in the PR description above. Before going into detail in your code comments, maybe let's first discuss them in general here:
If I'm not mistaken, we already talked about all of these in person but it's probably a good idea to also get this into writing here on Github: The order above is (most likely) also the order of their relevancy (from low to high) and unfortunately also the order of the incurred cost/complexity (from low to high). To my knowledge currently there exist no semi-directional architectures in practice. However, our heuristic mapper is implemented abstractly enough, that allowing for such architectures comes almost for free (2 extra jump instructions per search node (for purely bi-/unidirectional architectures), a boolean field User-defined gate costs and arbitrary 2q gates are probably much more relevant, but also more complex, which I only realized recently. As mentioned in the PR description, even the current implementation strictly speaking wrongly assumes a convex cost space due to cumulative reversal costs on logical edges possibly surpassing swap costs (in the non-fidelity-aware case on non-bidirectional architectures). E.g. if there are 9 congruent 2q-gates in a layer, which are already validly mapped to a back-edge incurring |
Thanks for the detailed answer. This cleared up quite a lot. I'll also start here before going in-depth with the comments.
Now that makes a lot more sense! Although such architecture do not exist in practice at the moment, it could very much happen and it almost never hurts to be a little more general. So I am happy with the changes here! Even better to hear, that the review helped in identifying the remaining places that needed changes.
I have feared as much. Although I still hope that the number of cases that need to be added to handle arbitrary two-qubit gates stays reasonably low. In fact, for bidirectional architectures there shouldn't be too many changes at all. There might be some more optimization potential (e.g., commutation rules involving controlled gates), but we are not taking advantage of that at the moment anyway. The unidirectional case might be trickier, but I think the existing PR already laid a solid foundation for that.
I agree that this mostly seems like a theoretical issue for now. Especially since I would guess that the cost difference between single- and two-qubit gates will always stay rather big. |
I agree, pre-computing all the distance values for common architectures (and maybe even a system for saving them to a file for custom architectures) is a great idea and would open the possibility of using metrics that are otherwise too expensive to compute for each mapping process. Too bad, the same is not possible/useful for fidelity distances because of their variability over time. Pre-computing viable swap sharing combinations would solve so much of the gap between the heuristic and true cost there. |
Although fidelity data varies over time, it all depends on the frequency of calibrations whether it is worth to invest the time and pre-compute these values. |
Hm, interesting point. I guess if you would map multiple circuits per calibration cycle, that's true, yes. I will keep it as an option in mind then👍 |
Description
This PR strives to solve multiple problems (that are just too interconnected to keep them to separate PRs):
GateCountSumDistanceMinusSharedSwaps
andGateCountMaxDistanceOrSumDistanceMinusSharedSwaps
, where the former is the sum of all qubit pair distances minus an upper bound for how many swaps could potentially be saved by sharing with other moving qubits, and the latter is just the dominating heuristic over the previousGateCountMaxDistance
and the newGateCountSumDistanceMinusSharedSwaps
.Tested on a subset of MQTbench and mapping to IBM Brisbane
GateCountMaxDistanceOrSumDistanceMinusSharedSwaps
reduces the effective branching rate by about 10% compared toGateCountMaxDistance
, while of course yielding the same results (if lookahead is disabled) as both heuristics are principally admissible.Unfortunately, the current default lookahead heuristic
GateCountMaxDistance
seems to not work well with this new heuristic, as the combination results in slightly higher costs. This is probably due to scaling issues (since the new heuristic is closer to the real cost and therefore larger, reducing the impact of the lookahead penalty) but might be solvable with higher lookahead factors or a new better suited lookahead heuristic.HeuristicMapper::Node
toHeuristicMapper
(since they grew more and more dependent on data structures fromHeuristicMapper
with recent PRs) and making them more atomic (i.e. reducing points at which nodes are in an inconsistent state)List of minor changes and bug fixes:
HeuristicMapper::Node
:nswaps
because of redundancy withswaps.size()
swaps
a one-dimensional vector (I assume the inner dimension was originally intended for the possibility of adding multiple swaps per node. However, this is currently not implemented, so all inner vectors are just of length 1)done
tovalidMapping
, which better describes the property now that non-tight heuristics have been introduced to QMAPHeuristicMapper::Node::validMappedTwoQubitGates
and activated it also for non-fidelity-aware heuristics enabling a more efficient check if a node has a valid mapping.operator>
), where previously all validly mapped nodes with the same total cost were considered equal (resulting in an arbitrary ordering in the priority queue and thereby potentially different optimal solutions for different principally admissible heuristics)HeuristicMapper::createInitialMapping
, that causes this function to go into an endless loop in some edge cases (instead of randomly iterating over the whole coupling map, it only iterates over a part of it due to the upper limit for the RNG being too low, which causes an endless loop once that subgraph is fully mapped but free teleportation qubits remain)Mapper::fidelities
Dijkstra::buildEdgeSkipTable
by internally callingDijkstra::buildTable
for the 0th dimensionHeuristicMapper::map
to new methodHeuristicMapper::checkParameters
Checklist: