Move Sycamore and Sycamore23 to GridDevice. #5544
Conversation
verult
requested review from
wcourtney,
a team,
vtomole and
cduck
as code owners
…factor/replace-sycamore-device
verult
force-pushed
the
cg-device-refactor/replace-sycamore-device
branch
from
42cba3e
to
f8767be
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -58,11 +60,9 @@ def create_device_proto_from_diagram( | |||
| durations_picos: Optional[Dict[str, int]] = None, | |||
| out: Optional[device_pb2.DeviceSpecification] = None, | |||
| ) -> device_pb2.DeviceSpecification: | |||
| """Parse ASCIIart device layout into DeviceSpecification proto. | |||
|
|
|||
| """[Deprecated] Parse ASCIIart device layout into DeviceSpecification proto. | |||
There was a problem hiding this comment.
I don't understand why we don't have a @_compat.deprecated decorator on this function. It should just be one line, so I don't think it will be burdensome at all, and it is much more obvious than a docstring.
There was a problem hiding this comment.
Sounds good. Tests need to include assert_deprecation() contexts but there aren't many of them. Will update
_SYCAMORE_DURATIONS would be trickier to deprecate, though, since it's a constant. Might need something similar to examples in PEP 562 and probably not worth the effort.
There was a problem hiding this comment.
That's fine. My comment was mainly about the function. _SYCAMORE_DURATIONS is a constant and it is already designated as private from the underscore, so I do not think we need a deprecation warning for it.
| @@ -71,7 +71,7 @@ | |||
| " num_circuits=num_circuits,\n", | |||
| " depth=depth,\n", | |||
| " num_qubits=depth,\n", | |||
| " device_graph=routing.gridqubits_to_graph_device(device.qubits),\n", | |||
| " device_graph=routing.gridqubits_to_graph_device(device.metadata.qubit_set),\n", | |||
There was a problem hiding this comment.
Optional: I wonder if it would be worth having a deprecated qubits() function in GridDevice to point to metadata.qubit_set? This would help people know how to upgrade this everywhere. This would be a separate PR of course. Just an idea.
There was a problem hiding this comment.
Good idea, will do!
| @@ -52,17 +55,16 @@ def _parse_device(s: str) -> Tuple[List[cirq.GridQubit], Dict[str, Set[cirq.Grid | |||
| return qubits, measurement_lines | |||
|
|
|||
|
|
|||
| @_compat.deprecated(deadline='v0.16', fix='This function will no longer be available.') | |||
There was a problem hiding this comment.
Should the fix be to use def create_device_proto_for_qubits?
verult
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Blocking SerializableDevice deprecation (quantumlib#5522) Recirq needs to be updated to use `Sycamore.metadata.qubit_set` in a follow-up. I put deprecation warnings as docstrings/comments since I don't expect them to have much usage outside Cirq (one exception being `_SYCAMORE_DURATION_PICOS` is used on server side to serialize DeviceSpecification the old way). Let me know if you'd prefer a full deprecation warning. @dstrain115 cc @MichaelBroughton @mpharrigan
Blocking SerializableDevice deprecation (#5522)
Recirq needs to be updated to use
Sycamore.metadata.qubit_setin a follow-up.I put deprecation warnings as docstrings/comments since I don't expect them to have much usage outside Cirq (one exception being
_SYCAMORE_DURATION_PICOSis used on server side to serialize DeviceSpecification the old way). Let me know if you'd prefer a full deprecation warning.@dstrain115
cc @MichaelBroughton @mpharrigan