Add metadata property for cirq_google devices. #4869
Conversation
MichaelBroughton
requested review from
cduck,
vtomole,
wcourtney and
a team
as code owners
| if len(pair) == 2 and pair[0] < pair[1] | ||
| ], | ||
| cirq.Gateset( | ||
| *[g for g in gate_definitions.keys() if isinstance(g, (cirq.Gate, type(cirq.Gate)))] |
There was a problem hiding this comment.
Why is this (cirq.Gate, type(cirq.Gate))? This seems redundant but I assume there's a good reason for it?
There was a problem hiding this comment.
Full disclosure: Things in this module are in pretty rough shape. In tests, the type contracts are somehow getting violated in this constructor and other "non Type[_Gate]" things are making it into this function. I'm working on deprecating qid_pairs in cirq-core and there are a few hangups in cirq_google that this PR will help unblock.
High level is I want to make sure we keep cirq-core pristine and cirq-google devices limping along in whatever weird state it is in so that we can finish the items here #4744 and then make a more focused effort on cleaning this up knowing that cirq-core now gives a solid foundation.
This weirdness keeps cirq-google behaving, albeit weirdly.
RE: backwards compatibility and changing in the future we should make a more focused effort on SerializableDevice before we cut our next release, I just don't think in this PR as things are bound to keep changing a lot here before that next release.
| cirq.Gateset( | ||
| *[g for g in gate_definitions.keys() if isinstance(g, (cirq.Gate, type(cirq.Gate)))] | ||
| ), | ||
| None, |
There was a problem hiding this comment.
I think the gate durations exist currently, so I think we could populate this if we wanted to.
There was a problem hiding this comment.
We could, but I think it's best left for when we can turn 100% of our attention to cleaning up things for SerializableDevice this PR is a means to an end for getting cleaner deprecations happening in cirq-core for qid_pairs.
| (pair[0], pair[1]) | ||
| for gate_defs in gate_definitions.values() | ||
| for gate_def in gate_defs | ||
| if gate_def.number_of_qubits == 2 |
There was a problem hiding this comment.
Just wondering: this is just a defensive check right? It should never be false?
There was a problem hiding this comment.
Not sure, I just copied this from lower down in the module qid_pairs function.
|
My concerns are resolved - leaving further review to other reviewers. |
|
LGTM, and changing the minimum to unblock cirq-core changes and revisiting in my project sgtm. |
MichaelBroughton
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
|
Automerge cancelled: A status check is failing. |
CirqBot
removed
automerge
…rq into cirq_google_metadata
MichaelBroughton
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
the
automerge
CirqBot
removed
the
front_of_queue_automerge
Adds metadata properties to cirq_google devices. Part of quantumlib#4743 . Note for @dstrain115 and @verult : I don't expect these metadata properties to take this exact form forever and that we may end up changing things once we have a more firmed up plan for how DeviceSpec protos and serialization might change.
Adds metadata properties to cirq_google devices. Part of #4743 .
Note for @dstrain115 and @verult : I don't expect these metadata properties to take this exact form forever and that we may end up changing things once we have a more firmed up plan for how DeviceSpec protos and serialization might change.