-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add isolated qubit functionality for gridmetadata. #5001
Add isolated qubit functionality for gridmetadata. #5001
Conversation
cirq-core/cirq/protocols/json_test_data/GridDeviceMetadata.json
Outdated
Show resolved
Hide resolved
did you consider having the arguments be g = nx.Graph()
g.add_edges_from(edgelist)
g.add_nodes_from(nodelist that may or may not include nodes in edges) |
if (b, a) not in edge_set: | ||
edge_set.add((a, b)) | ||
|
||
if isolated_qubits is None: |
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.
What if we default isolated_qubits
to an empty collection instead of None?
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.
Using empty collections is a dangerous default.
Here's a simplified example:
def f(value, key, store={}):
store[value] = key
return store
print(f('a', 1))
print(f('b', 2))
prints:
{'a': 1}
{'a': 1, 'b': 2}
Using it as None and then promoting it is a safer route that's recommended in the style guide.
@@ -39,12 +39,14 @@ def __init__( | |||
qubit_pairs: Iterable[Tuple['cirq.Qid', 'cirq.Qid']], | |||
gateset: 'cirq.Gateset', | |||
gate_durations: Optional[Dict['cirq.GateFamily', 'cirq.Duration']] = None, | |||
isolated_qubits: Optional[Iterable['cirq.Qid']] = None, |
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.
WDYT about using all_qubits
instead?
Pros
DeviceSpecification
keeps a full list of qubits, so the translation is easier. Current design plans to keep this field. This will minimize changes on Pyle side.- It will simplify the logic of computing the node set a bit, and also computing
all_qubits
for the super class constructor.
Cons
- If we want to expose an
isolated_qubits
property, still need to calculate that separately. This somewhat voids the benefit of reducing code for computingall_qubits
.
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.
Just saw @mpharrigan 's comment :P
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.
Changed over to all_qubits approach. PTAL.
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 for explaining the None default! LGTM
for q in isolated_qubits: | ||
if q in node_set: | ||
all_qubits = frozenset(all_qubits) | ||
for q in node_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.
nit: can be in the else
block of if all_qubits is None
.
Oops looks like @verult isn't a code owner for cirq-core. We should probably fix that. In the meantime, would @95-martin-orion you mind approving as well ? |
Adds isolated qubit property to GridDeviceMetadata. Also adds error checking for self loops. cc: @verult .
Adds isolated qubit property to GridDeviceMetadata. Also adds error checking for self loops. cc: @verult .
Adds isolated qubit property to GridDeviceMetadata. Also adds error checking for self loops. cc: @verult .
Adds isolated qubit property to GridDeviceMetadata. Also adds error checking for self loops. cc: @verult .