Rename GridDevice to GoogleGridDevice #5476
Conversation
| @@ -0,0 +1 @@ | |||
| cirq_google.GoogleGridDevice(cirq.GridDeviceMetadata(frozenset({(cirq.GridQubit(0, 1), cirq.GridQubit(0, 2)), (cirq.GridQubit(1, 0), cirq.GridQubit(1, 1)), (cirq.GridQubit(0, 0), cirq.GridQubit(1, 0)), (cirq.GridQubit(0, 1), cirq.GridQubit(1, 1)), (cirq.GridQubit(0, 0), cirq.GridQubit(0, 1)), (cirq.GridQubit(1, 1), cirq.GridQubit(1, 2)), (cirq.GridQubit(0, 2), cirq.GridQubit(1, 2))}), cirq.Gateset(cirq.ops.common_gates.XPowGate, cirq.ops.common_gates.YPowGate, cirq.ops.common_gates.ZPowGate, cirq.CZ, (cirq.ISWAP**0.5), unroll_circuit_op = True), {cirq.GateFamily(gate=cirq.ops.common_gates.XPowGate, ignore_global_phase=True, tags_to_accept=frozenset(), tags_to_ignore=frozenset()): cirq.Duration(nanos=1), cirq.GateFamily(gate=cirq.ops.common_gates.YPowGate, ignore_global_phase=True, tags_to_accept=frozenset(), tags_to_ignore=frozenset()): cirq.Duration(picos=1), cirq.GateFamily(gate=cirq.ops.common_gates.ZPowGate, ignore_global_phase=True, tags_to_accept=frozenset(), tags_to_ignore=frozenset()): cirq.Duration(picos=1), cirq.GateFamily(gate=cirq.CZ, ignore_global_phase=True, tags_to_accept=frozenset(), tags_to_ignore=frozenset()): cirq.Duration(picos=500), cirq.GateFamily(gate=(cirq.ISWAP**0.5), ignore_global_phase=True, tags_to_accept=frozenset(), tags_to_ignore=frozenset()): cirq.Duration(picos=600)}, frozenset({cirq.GridQubit(9, 9), cirq.GridQubit(0, 1), cirq.GridQubit(1, 1), cirq.GridQubit(0, 0), cirq.GridQubit(1, 2), cirq.GridQubit(10, 10), cirq.GridQubit(1, 0), cirq.GridQubit(0, 2)}), (cirq.CZTargetGateset(atol=1e-08, allow_partial_czs=False), cirq.SqrtIswapTargetGateset(atol=1e-08, required_sqrt_iswap_count=None, use_sqrt_iswap_inv=False)))) | |||
There was a problem hiding this comment.
optional: Maybe this could be a git file rename rather than a delete and readd if possible.
There was a problem hiding this comment.
I tried resetting both files to their original state and then running git mv + make the necessary changes. The diff still shows up as a delete and a readd.
AFAIU git doesn't track renames explicitly; rather it detects it automatically based on the diff. Perhaps the reason for failed detection in this case is because all lines of this file have been changed.
| @@ -33,7 +33,7 @@ def _class_resolver_dictionary() -> Dict[str, ObjectFactory]: | |||
| 'GoogleNoiseProperties': cirq_google.GoogleNoiseProperties, | |||
| 'SycamoreGate': cirq_google.SycamoreGate, | |||
| 'GateTabulation': cirq_google.GateTabulation, | |||
| 'GridDevice': cirq_google.GridDevice, | |||
| 'GoogleGridDevice': cirq_google.GoogleGridDevice, | |||
There was a problem hiding this comment.
This breaks old objects serialized with the GridDevice name. For backwards compatibility, the old mapping of 'GridDevice': GoogleGridDevice must also be preserved.
There was a problem hiding this comment.
Does this mean we can never add a new serializable GridDevice in the future ? If that's the case, it looks like there might be little value in doing this renaming from GridDevice --> GoogleGridDevice.
There was a problem hiding this comment.
Does this mean we can never add a new serializable
GridDevicein the future ?
That's how I understand it. For better or for worse, the change to using class names as the cirq_type locks us out of recycling names indefinitely.
There was a problem hiding this comment.
@verult Do you still think there's value in doing this rename? I'm fine with leaving it as GridDevice since it's already part of the cirq_google namespace so the "Google" part of "GoogleGridDevice" is implied. I think the rename would be more valuable if and when we have a conflicting cirq.GridDevice and cirq_google.GridDevice.
There was a problem hiding this comment.
As discussed offline with @95-martin-orion , unfortunately we don't include the module name in our JSON serialization, so if there is ever a cirq.GridDevice, it would be indistinguishable from the cirq_google one for JSON deserialization. So yes, I'll move forward with this after addressing Orion's serialization backward compatibility concerns.
There was a problem hiding this comment.
Synced with Cheng - the rename is safe since this is all happening within a single release cycle (0.14-0.15).
There was a problem hiding this comment.
Synced with @95-martin-orion on the backward compatibility issue. Since GridDevice did not exist prior to the last release, it won't be necessary to keep an extra backward compatible mapping.
|
Discussed with @95-martin-orion and @tanujkhattar offline. Ideally cirq-google JSON serializations should always be in the |
Preferred alternative over #5476 (renaming `GridDevice` to `GoogleGridDevice`). This resolves the issue of JSON name collisions between cirq_google and elsewhere by including the `cirq.google` namespace. Furthermore, by staying with `GridDevice` as the class name, we require the caller to include the `cirq-google` module name in cases when multiple `GridDevice` classes are used (for example in the server side code today). @95-martin-orion @mpharrigan cc @dstrain115 @tanujkhattar
…#5512) Preferred alternative over quantumlib#5476 (renaming `GridDevice` to `GoogleGridDevice`). This resolves the issue of JSON name collisions between cirq_google and elsewhere by including the `cirq.google` namespace. Furthermore, by staying with `GridDevice` as the class name, we require the caller to include the `cirq-google` module name in cases when multiple `GridDevice` classes are used (for example in the server side code today). @95-martin-orion @mpharrigan cc @dstrain115 @tanujkhattar
…#5512) Preferred alternative over quantumlib#5476 (renaming `GridDevice` to `GoogleGridDevice`). This resolves the issue of JSON name collisions between cirq_google and elsewhere by including the `cirq.google` namespace. Furthermore, by staying with `GridDevice` as the class name, we require the caller to include the `cirq-google` module name in cases when multiple `GridDevice` classes are used (for example in the server side code today). @95-martin-orion @mpharrigan cc @dstrain115 @tanujkhattar
To avoid confusion with the
GridDeviceMetadatawhich is in cirq-core instead of cirq-google. Don't have a strong preference either way, though.