Addressed Doug's comments

Author: verult

cirq-core/cirq/devices/grid_device_metadata.py

@@ -170,6 +170,6 @@ def _from_json_dict_(cls, qubit_pairs, gateset, gate_durations, all_qubits, **kw
         return cls(
             qubit_pairs,
             gateset,
-            None if gate_durations is None else dict(gate_durations),
+            dict(gate_durations) if gate_durations is not None else None,
             all_qubits,
         )

cirq-google/cirq_google/api/v2/device.proto

@@ -22,6 +22,7 @@ message DeviceSpecification {
   // A list of allowed ids for qubits within the Program.
   // Any programs with ids not in this list will be rejected.
   // If empty, all qubit values are allowed (e.g. in a simulator)
+  // Only grid qubits are supported. Strings must be in the form '<int>_<int>'.
   repeated string valid_qubits = 2;
 
   // A list of targets that gates can use.

cirq-google/cirq_google/devices/grid_device.py

@@ -14,6 +14,8 @@
 
 """Device object representing Google devices with a grid qubit layout."""
 
+import re
+
 from typing import Any, Set, Tuple, cast
 import cirq
 from cirq_google.api import v2
@@ -27,9 +29,16 @@ def _validate_device_specification(proto: v2.device_pb2.DeviceSpecification) ->
 
     Raises:
         ValueError: If the DeviceSpecification is invalid.
-
     """
 
+    # Qubit names must be in the form <int>_<int> to be parsed as cirq.GridQubits.
+    for q_name in proto.valid_qubits:
+        if re.match(r'^[0-9]+\_[0-9]+$', q_name) is None:
+            raise ValueError(
+                f"Invalid DeviceSpecification: valid_qubits contains the qubit '{q_name}' which is"
+                " not in the GridQubit form '<int>_<int>."
+            )
+
     for target_set in proto.valid_targets:
 
         # Check for unknown qubits in targets.
@@ -79,16 +88,16 @@ class GridDevice(cirq.Device):
     Example use cases:
 
         * Get an instance of a Google grid device.
-        >>> device = cirq_google.get_engine().get_processor('weber').get_device()
+        >>> device = cirq_google.get_engine().get_processor('processor_name').get_device()
 
         * Print the grid layout of the device.
         >>> print(device)
 
         * Determine whether a circuit can be run on the device.
-        >>> device.validate_circuit(circuit)  # Raises an exception if the circuit is invalid.
+        >>> device.validate_circuit(circuit)  # Raises a ValueError if the circuit is invalid.
 
         * Determine whether an operation can be run on the device.
-        >>> device.validate_operation(operation)  # Raises an exception if the operation is invalid.
+        >>> device.validate_operation(operation)  # Raises a ValueError if the operation is invalid.
 
         * Get the `cirq.Gateset` containing valid gates for the device, and inspect the full list
           of valid gates.
@@ -116,7 +125,7 @@ class GridDevice(cirq.Device):
 
     For Google devices, the
     [DeviceSpecification proto](
-        https://github.com/quantumlib/Cirq/blob/3969c2d3964cea56df33b329f036ba6810683882/cirq-google/cirq_google/api/v2/device.proto#L13
+        https://github.com/quantumlib/Cirq/blob/master/cirq-google/cirq_google/api/v2/device.proto
     )
     is the main specification for device information surfaced by the Quantum Computing Service.
     Thus, this class is should be instantiated using a `DeviceSpecification` proto via the
@@ -134,21 +143,29 @@ def __init__(self, metadata: cirq.GridDeviceMetadata):
     def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GridDevice':
         """Create a `GridDevice` from a DeviceSpecification proto.
 
-        This class only supports `cirq.GridQubit`s and `cirq.NamedQubit`s. If a
-        `DeviceSpecification.valid_qubits` string is in the form `<int>_<int>`, it is parsed as a
-        GridQubit. Otherwise it is parsed as a NamedQubit.
-
         Args:
             proto: The `DeviceSpecification` proto describing a Google device.
 
         Raises:
-            ValueError: If the given `DeviceSpecification` is invalid.
+            ValueError: If the given `DeviceSpecification` is invalid. It is invalid if:
+                * A `DeviceSpecification.valid_qubits` string is not in the form `<int>_<int>`, thus
+                  cannot be parsed as a `cirq.GridQubit`.
+                * `DeviceSpecification.valid_targets` refer to qubits which are not in
+                  `DeviceSpecification.valid_qubits`.
+                * A target set in `DeviceSpecification.valid_targets` has type `SYMMETRIC` or
+                  `ASYMMETRIC` but contains targets with repeated qubits, e.g. a qubit pair with a
+                  self loop.
+                * A target set in `DeviceSpecification.valid_targets` has type `SUBSET_PERMUTATION`
+                  but contains targets which do not have exactly one element. A `SUBSET_PERMUTATION`
+                  target set uses each target to represent a single qubit, and a gate can apply to
+                  any subset of qubits in the target set.
+
         """
 
         _validate_device_specification(proto)
 
         # Create qubit set
-        all_qubits = [_qid_from_str(q) for q in proto.valid_qubits]
+        all_qubits = [v2.grid_qubit_from_proto_id(q) for q in proto.valid_qubits]
 
         # Create qubit pair set
         #
@@ -159,7 +176,7 @@ def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GridDevice':
         # * Measurement gate can always be applied to all subset of qubits.
         #
         qubit_pairs = [
-            (_qid_from_str(target.ids[0]), _qid_from_str(target.ids[1]))
+            (v2.grid_qubit_from_proto_id(target.ids[0]), v2.grid_qubit_from_proto_id(target.ids[1]))
             for ts in proto.valid_targets
             for target in ts.targets
             if len(target.ids) == 2 and ts.target_ordering == v2.device_pb2.TargetSet.SYMMETRIC
@@ -212,36 +229,30 @@ def validate_operation(self, operation: cirq.Operation) -> None:
             raise ValueError(f'Qubit pair is not valid on device: {operation.qubits!r}')
 
     def __str__(self) -> str:
-        # If all qubits are grid qubits, render an appropriate text diagram.
-        if all(isinstance(q, cirq.GridQubit) for q in self._metadata.qubit_set):
-            diagram = cirq.TextDiagramDrawer()
+        diagram = cirq.TextDiagramDrawer()
 
-            qubits = cast(Set[cirq.GridQubit], self._metadata.qubit_set)
+        qubits = cast(Set[cirq.GridQubit], self._metadata.qubit_set)
 
-            # Don't print out extras newlines if the row/col doesn't start at 0
-            min_col = min(q.col for q in qubits)
-            min_row = min(q.row for q in qubits)
+        # Don't print out extras newlines if the row/col doesn't start at 0
+        min_col = min(q.col for q in qubits)
+        min_row = min(q.row for q in qubits)
 
-            for q in qubits:
-                diagram.write(q.col - min_col, q.row - min_row, str(q))
+        for q in qubits:
+            diagram.write(q.col - min_col, q.row - min_row, str(q))
 
-            # Find pairs that are connected by two-qubit gates.
-            Pair = Tuple[cirq.GridQubit, cirq.GridQubit]
-            pairs = sorted({cast(Pair, tuple(pair)) for pair in self._metadata.qubit_pairs})
+        # Find pairs that are connected by two-qubit gates.
+        Pair = Tuple[cirq.GridQubit, cirq.GridQubit]
+        pairs = sorted({cast(Pair, tuple(pair)) for pair in self._metadata.qubit_pairs})
 
-            # Draw lines between connected pairs. Limit to horizontal/vertical
-            # lines since that is all the diagram drawer can handle.
-            for q1, q2 in pairs:
-                if q1.row == q2.row or q1.col == q2.col:
-                    diagram.grid_line(
-                        q1.col - min_col, q1.row - min_row, q2.col - min_col, q2.row - min_row
-                    )
+        # Draw lines between connected pairs. Limit to horizontal/vertical
+        # lines since that is all the diagram drawer can handle.
+        for q1, q2 in pairs:
+            if q1.row == q2.row or q1.col == q2.col:
+                diagram.grid_line(
+                    q1.col - min_col, q1.row - min_row, q2.col - min_col, q2.row - min_row
+                )
 
-            return diagram.render(
-                horizontal_spacing=3, vertical_spacing=2, use_unicode_characters=True
-            )
-
-        return super().__str__()
+        return diagram.render(horizontal_spacing=3, vertical_spacing=2, use_unicode_characters=True)
 
     def _repr_pretty_(self, p: Any, cycle: bool) -> None:
         """Creates ASCII diagram for Jupyter, IPython, etc."""
@@ -260,15 +271,3 @@ def _from_json_dict_(cls, metadata, **kwargs):
 
     def _value_equality_values_(self):
         return self._metadata
-
-
-def _qid_from_str(id_str: str) -> cirq.Qid:
-    """Translates a qubit id string info cirq.Qid objects.
-
-    Tries to translate to GridQubit if possible (e.g. '4_3'), otherwise
-    falls back to using NamedQubit.
-    """
-    try:
-        return v2.grid_qubit_from_proto_id(id_str)
-    except ValueError:
-        return v2.named_qubit_from_proto_id(id_str)

cirq-google/cirq_google/devices/grid_device_test.py

@@ -80,8 +80,20 @@ def _create_device_spec_with_all_couplings():
     return grid_qubits, spec
 
 
+def _create_device_spec_invalid_qubit_name() -> v2.device_pb2.DeviceSpecification:
+    """Creates a DeviceSpecification with a qubit name that does not conform to '<int>_<int>'."""
+    q_proto_id = v2.qubit_to_proto_id(cirq.NamedQubit('q0_0'))
+
+    spec = v2.device_pb2.DeviceSpecification()
+    spec.valid_qubits.extend([q_proto_id])
+
+    return spec
+
+
 def _create_device_spec_qubit_pair_self_loops() -> v2.device_pb2.DeviceSpecification:
-    q_proto_id = v2.qubit_to_proto_id(cirq.NamedQubit('q'))
+    """Creates an invalid DeviceSpecification with a qubit pair ('0_0', '0_0')."""
+
+    q_proto_id = v2.qubit_to_proto_id(cirq.GridQubit(0, 0))
 
     spec = v2.device_pb2.DeviceSpecification()
     spec.valid_qubits.extend([q_proto_id])
@@ -95,6 +107,8 @@ def _create_device_spec_qubit_pair_self_loops() -> v2.device_pb2.DeviceSpecifica
 
 
 def _create_device_spec_invalid_qubit_in_qubit_pair() -> v2.device_pb2.DeviceSpecification:
+    """Creates a DeviceSpecification where qubit '0_1' is in a pair but not in valid_qubits."""
+
     q_proto_ids = [v2.qubit_to_proto_id(cirq.GridQubit(0, i)) for i in range(2)]
 
     spec = v2.device_pb2.DeviceSpecification()
@@ -109,6 +123,8 @@ def _create_device_spec_invalid_qubit_in_qubit_pair() -> v2.device_pb2.DeviceSpe
 
 
 def _create_device_spec_invalid_subset_permutation_target() -> v2.device_pb2.DeviceSpecification:
+    """Creates a DeviceSpecification where a SUBSET_PERMUTATION target contains 2 qubits."""
+
     q_proto_ids = [v2.qubit_to_proto_id(cirq.GridQubit(0, i)) for i in range(2)]
 
     spec = v2.device_pb2.DeviceSpecification()
@@ -122,7 +138,7 @@ def _create_device_spec_invalid_subset_permutation_target() -> v2.device_pb2.Dev
     return spec
 
 
-def test_grid_device_from_proto_and_validation():
+def test_grid_device_from_proto():
     grid_qubits, spec = _create_device_spec_with_horizontal_couplings()
 
     device = cirq_google.GridDevice.from_proto(spec)
@@ -165,6 +181,11 @@ def test_grid_device_validate_operations_negative():
     # TODO(#5050) verify validate_operations gateset errors
 
 
+def test_grid_device_invalid_qubit_name():
+    with pytest.raises(ValueError, match='not in the GridQubit form'):
+        cirq_google.GridDevice.from_proto(_create_device_spec_invalid_qubit_name())
+
+
 def test_grid_device_invalid_qubit_in_qubit_pair():
     with pytest.raises(ValueError, match='which is not in valid_qubits'):
         cirq_google.GridDevice.from_proto(_create_device_spec_invalid_qubit_in_qubit_pair())
@@ -218,11 +239,3 @@ def test_grid_device_repr_pretty(cycle, func):
     printer = mock.Mock()
     device._repr_pretty_(printer, cycle)
     printer.text.assert_called_once_with(func(device))
-
-
-def test_grid_device_str_named_qubits():
-    q_proto_id = v2.qubit_to_proto_id(cirq.NamedQubit('q'))
-    spec = v2.device_pb2.DeviceSpecification()
-    spec.valid_qubits.extend([q_proto_id])
-    device = cirq_google.GridDevice.from_proto(spec)
-    assert device.__class__.__name__ in str(device)