jvansomeren
commented
1 year ago Yes, given the current all-to-all connectivity of multi-core. As soon as we deviate from this, get_core_distance comes back. Having a grid of cores was in my mind when originally writing this. So I’m ok with rewriting this to a bool but bear in mind that it will come back, probably soon.
Op 3 mei 2023, om 12:19 heeft Pablo Le Hénaff @.**@.>> het volgende geschreven:
@pablolh commented on this pull request.
In src/ql/com/topology.cchttps://urldefense.com/v3/__https://github.com/QuTech-Delft/OpenQL/pull/478*discussion_r1183465478__;Iw!!PAKc-5URQlI!_9CSc-bfUB6VttmJQnLa-ASjEGdPagFDt9HliDxsHEmlEF8v031McRICXy25XpkOndbx_NVEoJ40DHDcryc8ZrzEyEeyNgz3$:
@@ -685,42 +689,73 @@ utils::UInt Topology::get_distance(Qubit source, Qubit target) const { } return d; }
return distance[source][target]; }
/**
- Returns the distance between the given two qubits in terms of cores. */ utils::UInt Topology::get_core_distance(Qubit source, Qubit target) const {
This should return a bool and be renamed "are_in_same_core" or something like that
In src/ql/com/topology.cchttps://urldefense.com/v3/__https://github.com/QuTech-Delft/OpenQL/pull/478*discussion_r1183469158__;Iw!!PAKc-5URQlI!_9CSc-bfUB6VttmJQnLa-ASjEGdPagFDt9HliDxsHEmlEF8v031McRICXy25XpkOndbx_NVEoJ40DHDcryc8ZrzEyIWyVHsn$:
*/ utils::UInt Topology::get_min_hops(Qubit source, Qubit target) const {
- utils::UInt d = get_distance(source, target);
- utils::UInt cd = get_core_distance(source, target);
- QL_ASSERT(cd <= d);
- if (cd == d) {
- return d+2;
- if (source == target) {
- return 0;
- }
- utils::UInt distance = get_distance(source, target);
- utils::UInt core_distance = get_core_distance(source, target);
- QL_ASSERT(core_distance <= distance);
- if (core_distance == distance) {
at this point, distance >= 1 and core_distance is only 0 or 1 so this "if" implies core_distance == distance == 1 and source and target being in different cores, therefore is_comm_qubit(source) and is_comm_qubit(target) are always true, no?
I would rewrite as such, which to me is more readable:
if (!are_in_same_core(source, target) && is_comm_qubit(source) && is_comm_qubit(target)) { QL_ASSERT(distance == 1); if (num_comm_qubits == 1) { return 3; } else { return 2; } }
and then core_distance variable is useless.
Btw I think we should throw an exception when "edges" key is present or "connectivitiy" is "specified" and num_cores >= 2, in light of what Roberto told me about the assumption with multi-core connectivity.
In src/ql/com/topology.cchttps://urldefense.com/v3/__https://github.com/QuTech-Delft/OpenQL/pull/478*discussion_r1183474582__;Iw!!PAKc-5URQlI!_9CSc-bfUB6VttmJQnLa-ASjEGdPagFDt9HliDxsHEmlEF8v031McRICXy25XpkOndbx_NVEoJ40DHDcryc8ZrzEyLK2uAQt$:
*
- We assume below that a valid path exists with distance+1 hops; this fails
- when not all qubits in a core support connections to all other cores.
- See the check in initialization of neighbors.
- Inside one core, minimum number of hops == distance.
- *
- Between qubits in different cores, the minimum number of hops >= distance + 1.
The distinction between "minimum number of hops" and distance is here very confusing. A distance is a minimum number of hops already. I think get_min_hops should be renamed with a name that has to do with routing.
— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/QuTech-Delft/OpenQL/pull/478*pullrequestreview-1410533057__;Iw!!PAKc-5URQlI!_9CSc-bfUB6VttmJQnLa-ASjEGdPagFDt9HliDxsHEmlEF8v031McRICXy25XpkOndbx_NVEoJ40DHDcryc8ZrzEyHV9i4q_$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AEDTBNVF4W6PLYWQ7IDNQF3XEIWLBANCNFSM6AAAAAAXUENLHE__;!!PAKc-5URQlI!_9CSc-bfUB6VttmJQnLa-ASjEGdPagFDt9HliDxsHEmlEF8v031McRICXy25XpkOndbx_NVEoJ40DHDcryc8ZrzEyKM34FCn$. You are receiving this because your review was requested.Message ID: @.***>
The main change of this pull request is in topology.cc, get_min_hops function:
distance + 1case is now taken into consideration.The other important change is in mapper.cc:
are_nearest_neighborsfunction to encapsulate theget_min_hops(source, target) == 1check there.Apart from this, other minor changes:
source == targetbase case out ofconnectivity == FULLat topology.cc, get_distance.source == targetbase case in topology.cc, get_core_distance.source == targetbase case in topology.cc, get_min_hops.