Skip to content
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

DeviceMetadata docs update. #4963

Merged
merged 6 commits into from
Feb 8, 2022

Conversation

MichaelBroughton
Copy link
Collaborator

Closes #4962

Comment on lines 240 to +241
def qubit_set(self) -> FrozenSet['cirq.Qid']:
"""Returns a set of qubits on the device, if possible.
"""Returns the set of qubits on the device.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So objects like _UnconstrainedDevice would no longer be possible to write?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still have the UnconstrainedDevice as it exists today. We just won't be able to add the metadata property to it because it doesn't have any qubits or nx_graph.

Comment on lines 80 to 86
if working_gatefamilies != gateset.gates:
missing_items = working_gatefamilies.difference(gateset.gates)
raise ValueError(
"Supplied gate_durations contains gates not present"
f" in supported_gates. {missing_items} in supported_gates"
f" in gateset. {missing_items} in gateset"
" is False."
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are checking for equality but reporting difference in the ValueError. It's possible that working_gatefamilies is a subset of gateset.gates, in which case missing_items will be an empty set and error message would not be useful.

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nits.

Comment on lines 82 to 83
"Supplied gate_durations contains gates not present"
" in gateset."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This is still not correct, because if gate_durations is a subset of gateset, it won't contain any gates not present in the gateset, but vice versa would be true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated wording here to reflect the fact we are doing strict equality checks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gate_durations.keys should be allowed to be a subset of the gateset, because a gateset could contain GateFamilies that don't represent actual gates, such as FSimGateFamily.

An alternative Gateset design is to separate out GateFamilies like FSimGateFamily into a separate property equivalence_gate_families, which are used somewhat differently compared to GateFamilies representing gates.

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 8, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 8, 2022
@CirqBot CirqBot merged commit 5eae221 into quantumlib:master Feb 8, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 8, 2022
95-martin-orion pushed a commit to 95-martin-orion/Cirq that referenced this pull request Mar 2, 2022
CirqBot pushed a commit that referenced this pull request Apr 30, 2022
… of gateset (#5309)

Fix for #4963 (comment)

In the current Gateset design, some GateFamilies in a Gateset may not correspond to an actual device gate (e.g. `cirq_google.FSimGateFamily`). The new `GridDevice` implementation includes `FSimGateFamily` in its gateset to accept all equivalent Cirq representations of valid 2-qubit gates.

@MichaelBroughton 
cc @tanujkhattar
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
… of gateset (quantumlib#5309)

Fix for quantumlib#4963 (comment)

In the current Gateset design, some GateFamilies in a Gateset may not correspond to an actual device gate (e.g. `cirq_google.FSimGateFamily`). The new `GridDevice` implementation includes `FSimGateFamily` in its gateset to accept all equivalent Cirq representations of valid 2-qubit gates.

@MichaelBroughton 
cc @tanujkhattar
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
… of gateset (quantumlib#5309)

Fix for quantumlib#4963 (comment)

In the current Gateset design, some GateFamilies in a Gateset may not correspond to an actual device gate (e.g. `cirq_google.FSimGateFamily`). The new `GridDevice` implementation includes `FSimGateFamily` in its gateset to accept all equivalent Cirq representations of valid 2-qubit gates.

@MichaelBroughton 
cc @tanujkhattar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix types / docstrings on DeviceMetadata
4 participants