-
Notifications
You must be signed in to change notification settings - Fork 48
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
Update LexiRoute::reassign_node
to use Ancilla Node
#715
Conversation
Boolean edges don't respect linearity, but bundles do. Remove extraneous code.
This reverts commit 5abfd98.
…ements""" This reverts commit fa93564.
This reverts commit e44f42a.
Hopefully reduce branching, at least make more readable
…ached to Input vertex
Leaving comments as waiting to see if CI passes before removing
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.
Looks good to me. Only a few comments. Also a changelog entry would be helpful.
} | ||
|
||
// "assignee" is Circuit UnitID being relabelled to UnitID "replacement" |
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.
Maybe document this in the header file? This seems to be the only method without a docstring.
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.
Good point, done
SCENARIO( | ||
"Test relabelling a Circuit UnitID that is an Architecture Node but " | ||
"reassignable to an Ancilla Node.") { | ||
GIVEN("Line Architecture, one reassignment.") { |
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.
Can you add some comments explaining why some UnitID have been reassigned to an ancilla? My next comment is related to this.
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.
Unfortunately the condition in which this occurs is rather niche. I've added a small explanation to the test:
• If a non-labelled Qubit in a Circuit being mapped has no Quantum gates with physical constraints (i.e. mostly multi-qubit gates) then before mapping we assign it some "bad" Architecture Node (typically something on the edge of the coupling graph with low out-degree)
• Any non-labelled Qubit in a Circuit with Quantum gates with physical constraints are left unlabelled
• During mapping, if a multi-qubit gate with a non-labelled Qubit is encountered we need to allocate it to some best Architecture Node
• In some cases, this best Node may end up being a "bad" Node we've used to assign an "unimportant" (without connectivity graph related physical constraints) Qubit too. If this is the case we relabel the unlabelled Qubit to this best Node and find a new Node to assign the "unimportant" Qubit to
• Ideally we just find a spare Architecture Node that hasn't previously been assigned to and reassign the "unimportant" Qubit to it. However in some cases there can be no spare Architecture Node as they have been used as ancilla Node for SWAP/BRIDGE gates
• This is the case that was not considered before - the assertion is basically saying "there should always be a spare Architecture Node" but this is not true (this isn't in the comments)
• In this case we take an Ancilla Node and wire its output to the input of the "unimportant" Qubit Path, essentially reassigning it.
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! this is much appreciated.
PassPtr r_p = gen_routing_pass( | ||
architecture, {std::make_shared<LexiLabellingMethod>(), | ||
std::make_shared<LexiRouteRoutingMethod>()}); | ||
REQUIRE(r_p->apply(cu)); |
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 think REQUIRE(r_p->apply(cu));
(also in the other new tests) doesn't say much. Are there anything else we can check to prevent future regression?
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.
Agreed, as essentially it's confirming no assertion is made. I can try and make it slightly more secure by checking the bimap returned during compilation has qubits relabelled as expected.
See TKET-2761. In Routing the
Qubit
UnitID
objects in aCircuit
are relabelled toNode
objects from theArchitecture
theCircuit
is being routed to be compatible with. There are scenarios where aQubit
is labelled as oneNode
, but we can freely relabel it to anotherNode
if theNode
it is already assigned to is more helpful being used elsewhere. In this case we need to relabel the firstQubit
to anotherNode
on theArchitecture
that is not already in use.When we do this, the logic in
LexiRoute::reassign_node
only checks forNode
in theArchitecture
that aren't in use and relabels to them. However, it is possible there are no "free"Node
in theArchitecture
as they have been used as ancillas. This PR updatesLexiRoute::reassign_node
to relabel to an ancillaNode
if there are no "free"Node
left to use.