-
Notifications
You must be signed in to change notification settings - Fork 2
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
Routing #6
Routing #6
Conversation
…ial-routing' branch
Here's a little tip in case you weren't aware: Use |
|
||
class TransportError(Exception): | ||
def __init__(self, a: list[int], b: list[int]): | ||
super().__init__(f"Traps different sizes: {len(a)} vs. {len(b)}") |
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.
Rather than different sizes, this is more telling that placement was done improperly. If the current state does not have the same number of qubits as the goal state, it implies that not every qubit was placed properly. It is a side effect of improper placement.
tests/jonhas_test.py
Outdated
|
||
m = TestMachine(12, {0,2,4,6,8,10}, 10, 2, 2) | ||
|
||
def parse_circuit(file): |
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.
Remove as we should only be working with pytket circuits now that Neal's sharding looks good
tests/test.py
Outdated
assert transport_cost(init, goal, swap_cost) == 2 | ||
|
||
#many | ||
init = [0,1,2,3,4,5,6,7,8,9] |
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.
If we are looking at distance based, the number of swaps isn't 9 here is it, but rather 11?
# Because SQ zones are offsets of TQ zones, each tq op covers 2 sq zones | ||
raise GateOpportunitiesError | ||
|
||
# place the tq ops |
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.
You may want to break the tq placement into a method to help make this function less long.
placed_qubits.add(q2) | ||
|
||
# place the sq ops | ||
for op in sq_ops: |
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.
Similarly to above a method for placing sq ops seems natural.
pytket/phir/placement.py
Outdated
raise GateOpportunitiesError | ||
|
||
# place the tq ops | ||
for op in tq_ops_sorted: |
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 looks quite similar to the tq placement code in the previous method. I didn't run a full diff, but it seems like there's a lot of shared elements, which makes me want to double down on the idea that the tq placement can be broken down into a helper method.
pytket/phir/machine_class.py
Outdated
""" | ||
self.size = size | ||
self.tq_options = tq_options | ||
self.sq_options = set() |
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 the argument types make the types of the various fields obvious, but we may still want to either add type annotations to each of the fields in the class, or at the least add the annotation self.sq_options: set[int] = set()
, so it's clear what thet set is supposed to hold.
tq_options = {4, 8, 12, 16, 20, 24, 28} | ||
sq_options = set(range(32)) | ||
trap_size = 32 | ||
expected = [ |
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.
how did you arrive at this expected test value? The way the tq gates map to tq_indices seems straightforward, but the rest does not.
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'm ok with merging things as they currently stand so that we can move quickly, but we should definitely do some cleanup passes before we more publicly advertise this tool to users.
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.
LGTM, there are some additional questions I had but I don't mind approving to be a bit more quicker on everything
No description provided.