Addressed Michael's comments

cirq-google/cirq_google/__init__.py

@@ -57,8 +57,8 @@
 from cirq_google.devices import (
     Bristlecone,
     Foxtail,
-    GoogleDevice,
     GoogleNoiseProperties,
+    GridDevice,
     NoiseModelFromGoogleNoiseProperties,
     SerializableDevice,
     Sycamore,

cirq-google/cirq_google/devices/__init__.py

@@ -19,7 +19,7 @@
 
 from cirq_google.devices.known_devices import Bristlecone, Foxtail, Sycamore, Sycamore23
 
-from cirq_google.devices.google_device import GoogleDevice
+from cirq_google.devices.grid_device import GridDevice
 
 from cirq_google.devices.serializable_device import SerializableDevice
 

cirq-google/cirq_google/devices/google_device.py → cirq-google/cirq_google/devices/grid_device.py

@@ -12,27 +12,73 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""Device object representing Google devices."""
+"""Device object representing Google devices with a grid qubit layout."""
 
 from typing import Any, Set, Tuple, cast
 import cirq
 from cirq_google.api import v2
 
 
+def _validate_device_specification(proto: v2.device_pb2.DeviceSpecification) -> None:
+    """Validates the DeviceSpecification proto.
+
+    Args:
+        proto: The DeviceSpecification proto to validate.
+
+    Raises:
+        ValueError: If the DeviceSpecification is invalid.
+
+    """
+
+    for target_set in proto.valid_targets:
+
+        # Check for unknown qubits in targets.
+        for target in target_set.targets:
+            for target_id in target.ids:
+                if target_id not in proto.valid_qubits:
+                    raise ValueError(
+                        f"Invalid DeviceSpecification: valid_targets contain qubit '{target_id}'"
+                        " which is not in valid_qubits."
+                    )
+
+        # Symmetric and asymmetric targets should not have repeated qubits.
+        if (
+            target_set.target_ordering == v2.device_pb2.TargetSet.SYMMETRIC
+            or target_set.target_ordering == v2.device_pb2.TargetSet.ASYMMETRIC
+        ):
+            for target in target_set.targets:
+                if len(target.ids) > len(set(target.ids)):
+                    raise ValueError(
+                        f"Invalid DeviceSpecification: the target set '{target_set.name}' is either"
+                        " SYMMETRIC or ASYMMETRIC but has a target which contains repeated qubits:"
+                        f" {target.ids}."
+                    )
+
+        # A SUBSET_PERMUTATION target should contain exactly one qubit.
+        if target_set.target_ordering == v2.device_pb2.TargetSet.SUBSET_PERMUTATION:
+            for target in target_set.targets:
+                if len(target.ids) != 1:
+                    raise ValueError(
+                        f"Invalid DeviceSpecification: the target set '{target_set.name}' is of"
+                        " type SUBSET_PERMUTATION but contains a target which does not have exactly"
+                        f" 1 qubit: {target.ids}."
+                    )
+
+
 @cirq.value_equality
-class GoogleDevice(cirq.Device):
-    """Device object representing Google devices.
+class GridDevice(cirq.Device):
+    """Device object representing Google devices with a grid qubit layout.
 
     For end users, instances of this class are typically accessed via
     `Engine.get_processor('processor_name').get_device()`.
 
     This class is compliant with the core `cirq.Device` abstraction. In particular:
         * Device information is captured in the `metadata` property.
-        * An instance of `GoogleDevice` can be used to validate circuits, moments, and operations.
+        * An instance of `GridDevice` can be used to validate circuits, moments, and operations.
 
     Example use cases:
 
-        * Get an instance of a Google device.
+        * Get an instance of a Google grid device.
         >>> device = cirq_google.get_engine().get_processor('weber').get_device()
 
         * Print the grid layout of the device.
@@ -78,15 +124,15 @@ class GoogleDevice(cirq.Device):
     """
 
     def __init__(self, metadata: cirq.GridDeviceMetadata):
-        """Creates a GoogleDevice object.
+        """Creates a GridDevice object.
 
         This constructor typically should not be used directly. Use `from_proto()` instead.
         """
         self._metadata = metadata
 
     @classmethod
-    def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':
-        """Create a `GoogleDevice` from a DeviceSpecification proto.
+    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
@@ -99,6 +145,8 @@ def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':
             ValueError: If the given `DeviceSpecification` is invalid.
         """
 
+        _validate_device_specification(proto)
+
         # Create qubit set
         all_qubits = [_qid_from_str(q) for q in proto.valid_qubits]
 
@@ -110,7 +158,6 @@ def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':
         # * All valid qubits work for all single-qubit gates.
         # * Measurement gate can always be applied to all subset of qubits.
         #
-        # TODO(#5169) Consider adding the reversed pair, depending on the issue's solution.
         qubit_pairs = [
             (_qid_from_str(target.ids[0]), _qid_from_str(target.ids[1]))
             for ts in proto.valid_targets
@@ -128,10 +175,10 @@ def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':
         except ValueError as ve:
             raise ValueError("DeviceSpecification is invalid.") from ve
 
-        return GoogleDevice(metadata)
+        return GridDevice(metadata)
 
     @property
-    def metadata(self):
+    def metadata(self) -> cirq.GridDeviceMetadata:
         """Get metadata information for the device."""
         return self._metadata
 
@@ -157,8 +204,10 @@ def validate_operation(self, operation: cirq.Operation) -> None:
             if q not in self._metadata.qubit_set:
                 raise ValueError(f'Qubit not on device: {q!r}')
 
-        # TODO(#5169) May need to check the reverse pair depending on the issue's solution.
-        if len(operation.qubits) == 2 and tuple(operation.qubits) not in self._metadata.qubit_pairs:
+        if (
+            len(operation.qubits) == 2
+            and frozenset(operation.qubits) not in self._metadata.qubit_pairs
+        ):
             raise ValueError(f'Qubit pair is not valid on device: {operation.qubits!r}')
 
     def __str__(self) -> str:
@@ -177,7 +226,7 @@ def __str__(self) -> str:
 
             # Find pairs that are connected by two-qubit gates.
             Pair = Tuple[cirq.GridQubit, cirq.GridQubit]
-            pairs = sorted({cast(Pair, pair) for pair in self._metadata.qubit_pairs})
+            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.
@@ -199,12 +248,10 @@ def _repr_pretty_(self, p: Any, cycle: bool) -> None:
         p.text(repr(self) if cycle else str(self))
 
     def __repr__(self) -> str:
-        return f'cirq_google.GoogleDevice({repr(self._metadata)})'
+        return f'cirq_google.GridDevice({repr(self._metadata)})'
 
     def _json_dict_(self):
-        return {
-            'metadata': self._metadata,
-        }
+        return {'metadata': self._metadata}
 
     @classmethod
     def _from_json_dict_(cls, metadata, **kwargs):

cirq-google/cirq_google/devices/google_device_test.py → cirq-google/cirq_google/devices/grid_device_test.py

@@ -33,14 +33,20 @@ def _create_device_spec_with_horizontal_couplings():
     #   x -- x
 
     grid_qubits = [cirq.GridQubit(i, j) for i in range(GRID_HEIGHT) for j in range(2)]
+
     spec = v2.device_pb2.DeviceSpecification()
     spec.valid_qubits.extend([v2.qubit_to_proto_id(q) for q in grid_qubits])
     grid_targets = spec.valid_targets.add()
     grid_targets.name = '2_qubit_targets'
     grid_targets.target_ordering = v2.device_pb2.TargetSet.SYMMETRIC
-    for row in range(GRID_HEIGHT):
+    for row in range(int(GRID_HEIGHT / 2)):
         new_target = grid_targets.targets.add()
         new_target.ids.extend([v2.qubit_to_proto_id(cirq.GridQubit(row, j)) for j in range(2)])
+    for row in range(int(GRID_HEIGHT / 2), GRID_HEIGHT):
+        # Flip the qubit pair order for the second half of qubits
+        # to verify GridDevice properly handles pair symmetry.
+        new_target = grid_targets.targets.add()
+        new_target.ids.extend([v2.qubit_to_proto_id(cirq.GridQubit(row, 1 - j)) for j in range(2)])
     gate = spec.valid_gates.add()
     gate.syc.SetInParent()
     gate.gate_duration_picos = 12000
@@ -74,7 +80,7 @@ def _create_device_spec_with_all_couplings():
     return grid_qubits, spec
 
 
-def _create_device_spec_with_qubit_pair_self_loops() -> v2.device_pb2.DeviceSpecification:
+def _create_device_spec_qubit_pair_self_loops() -> v2.device_pb2.DeviceSpecification:
     q_proto_id = v2.qubit_to_proto_id(cirq.NamedQubit('q'))
 
     spec = v2.device_pb2.DeviceSpecification()
@@ -88,7 +94,7 @@ def _create_device_spec_with_qubit_pair_self_loops() -> v2.device_pb2.DeviceSpec
     return spec
 
 
-def _create_device_spec_with_invalid_qubit_in_qubit_pair() -> v2.device_pb2.DeviceSpecification:
+def _create_device_spec_invalid_qubit_in_qubit_pair() -> v2.device_pb2.DeviceSpecification:
     q_proto_ids = [v2.qubit_to_proto_id(cirq.GridQubit(0, i)) for i in range(2)]
 
     spec = v2.device_pb2.DeviceSpecification()
@@ -102,22 +108,36 @@ def _create_device_spec_with_invalid_qubit_in_qubit_pair() -> v2.device_pb2.Devi
     return spec
 
 
-def test_google_device_from_proto_and_validation():
+def _create_device_spec_invalid_subset_permutation_target() -> v2.device_pb2.DeviceSpecification:
+    q_proto_ids = [v2.qubit_to_proto_id(cirq.GridQubit(0, i)) for i in range(2)]
+
+    spec = v2.device_pb2.DeviceSpecification()
+    spec.valid_qubits.extend(q_proto_ids)
+    targets = spec.valid_targets.add()
+    targets.name = 'test_targets'
+    targets.target_ordering = v2.device_pb2.TargetSet.SUBSET_PERMUTATION
+    new_target = targets.targets.add()
+    new_target.ids.extend(q_proto_ids)  # should only have 1 qubit instead
+
+    return spec
+
+
+def test_grid_device_from_proto_and_validation():
     grid_qubits, spec = _create_device_spec_with_horizontal_couplings()
 
-    device = cirq_google.GoogleDevice.from_proto(spec)
+    device = cirq_google.GridDevice.from_proto(spec)
 
     assert len(device.metadata.qubit_set) == len(grid_qubits)
     assert device.metadata.qubit_set == frozenset(grid_qubits)
     assert all(
-        (cirq.GridQubit(row, 0), cirq.GridQubit(row, 1)) in device.metadata.qubit_pairs
+        frozenset((cirq.GridQubit(row, 0), cirq.GridQubit(row, 1))) in device.metadata.qubit_pairs
         for row in range(GRID_HEIGHT)
     )
 
 
-def test_google_device_validate_operations_positive():
+def test_grid_device_validate_operations_positive():
     grid_qubits, spec = _create_device_spec_with_horizontal_couplings()
-    device = cirq_google.GoogleDevice.from_proto(spec)
+    device = cirq_google.GridDevice.from_proto(spec)
 
     for q in grid_qubits:
         device.validate_operation(cirq.X(q))
@@ -129,9 +149,9 @@ def test_google_device_validate_operations_positive():
     # TODO(#5050) verify validate_operations gateset support
 
 
-def test_google_device_validate_operations_negative():
+def test_grid_device_validate_operations_negative():
     grid_qubits, spec = _create_device_spec_with_horizontal_couplings()
-    device = cirq_google.GoogleDevice.from_proto(spec)
+    device = cirq_google.GridDevice.from_proto(spec)
 
     q = cirq.GridQubit(10, 10)
     with pytest.raises(ValueError, match='Qubit not on device'):
@@ -145,31 +165,32 @@ def test_google_device_validate_operations_negative():
     # TODO(#5050) verify validate_operations gateset errors
 
 
-@pytest.mark.parametrize(
-    'spec',
-    [
-        # TODO(#5050) implement once gateset support is implemented
-        # _create_device_spec_with_missing_gate_durations(),
-        _create_device_spec_with_qubit_pair_self_loops(),
-        _create_device_spec_with_invalid_qubit_in_qubit_pair(),
-    ],
-)
-def test_google_device_invalid_device_spec(spec):
-    with pytest.raises(ValueError, match='DeviceSpecification is invalid'):
-        cirq_google.GoogleDevice.from_proto(spec)
+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())
+
+
+def test_grid_device_invalid_target_self_loops():
+    with pytest.raises(ValueError, match='contains repeated qubits'):
+        cirq_google.GridDevice.from_proto(_create_device_spec_qubit_pair_self_loops())
+
+
+def test_grid_device_invalid_subset_permutation_target():
+    with pytest.raises(ValueError, match='does not have exactly 1 qubit'):
+        cirq_google.GridDevice.from_proto(_create_device_spec_invalid_subset_permutation_target())
 
 
-def test_google_device_repr_json():
+def test_grid_device_repr_json():
     _, spec = _create_device_spec_with_horizontal_couplings()
-    device = cirq_google.GoogleDevice.from_proto(spec)
+    device = cirq_google.GridDevice.from_proto(spec)
 
     assert eval(repr(device)) == device
     assert cirq.read_json(json_text=cirq.to_json(device)) == device
 
 
-def test_google_device_str_grid_qubits():
+def test_grid_device_str_grid_qubits():
     _, spec = _create_device_spec_with_all_couplings()
-    device = cirq_google.GoogleDevice.from_proto(spec)
+    device = cirq_google.GridDevice.from_proto(spec)
 
     assert (
         str(device)
@@ -191,17 +212,17 @@ def test_google_device_str_grid_qubits():
 
 
 @pytest.mark.parametrize('cycle,func', [(False, str), (True, repr)])
-def test_google_device_repr_pretty(cycle, func):
+def test_grid_device_repr_pretty(cycle, func):
     _, spec = _create_device_spec_with_all_couplings()
-    device = cirq_google.GoogleDevice.from_proto(spec)
+    device = cirq_google.GridDevice.from_proto(spec)
     printer = mock.Mock()
     device._repr_pretty_(printer, cycle)
     printer.text.assert_called_once_with(func(device))
 
 
-def test_serializable_device_str_named_qubits():
+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.GoogleDevice.from_proto(spec)
+    device = cirq_google.GridDevice.from_proto(spec)
     assert device.__class__.__name__ in str(device)

cirq-google/cirq_google/json_resolver_cache.py

@@ -33,7 +33,7 @@ def _class_resolver_dictionary() -> Dict[str, ObjectFactory]:
         'GoogleNoiseProperties': cirq_google.GoogleNoiseProperties,
         'SycamoreGate': cirq_google.SycamoreGate,
         'GateTabulation': cirq_google.GateTabulation,
-        'GoogleDevice': cirq_google.GoogleDevice,
+        'GridDevice': cirq_google.GridDevice,
         'PhysicalZTag': cirq_google.PhysicalZTag,
         'FSimGateFamily': cirq_google.FSimGateFamily,
         'FloquetPhasedFSimCalibrationOptions': cirq_google.FloquetPhasedFSimCalibrationOptions,