Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GridDevice serialization refactor #6094

Conversation

verult
Copy link
Collaborator

@verult verult commented May 12, 2023

This refactor adds the to_proto() method to serialize GridDevice into DeviceSpecification. The convenience method to create a DeviceSpecification from partial gateset information, _create_device_specification_proto(), is also reimplemented as GridDevice.from_device_information() and returns a GridDevice instead. These features makes using server-side GridDevice viable - Server-side and client-side GridDevice instances should now be identical.

In addition, gateset serialization and deserialization mappings are reorganized into a shared data structure.

cc @maffoo @pavoljuhas

@verult verult requested a review from dstrain115 May 12, 2023 21:23
@verult verult requested review from wcourtney, a team, vtomole and cduck as code owners May 12, 2023 21:23
@verult verult requested review from dstrain115 and removed request for dstrain115 May 12, 2023 21:24
@verult verult force-pushed the cg-device-refactor/griddevice-serialization-by-serializing-gateset branch from 3aa0c24 to 3cf2d1d Compare May 12, 2023 21:25
@verult verult force-pushed the cg-device-refactor/griddevice-serialization-by-serializing-gateset branch from 9910c79 to 4837660 Compare May 16, 2023 18:36
@CirqBot CirqBot added the size: L 250< lines changed <1000 label May 16, 2023
"""GateFamilies which match all other valid gate representations."""
additional_forms: List[cirq.GateFamily] = field(default_factory=list)
"""`primary_forms` (as GateFamilies) + `additional_forms`"""
all_forms: List[cirq.GateFamily] = field(init=False)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: Should this be a generated @Property instead? I guess, with init=False, they can't specify it, so maybe that's okay.

"""The name of gate type in `GateSpecification`."""
gate_spec_name: str
"""Gate representations to be included in generated gatesets and gate durations."""
primary_forms: List[GateOrFamily]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The primary and additional forms definition is kind of confusing to me. This new terminology is not explained very clearly by these docstrings. Maybe the class docstring could give more context as to what's going on here?

cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@verult verult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass at address comments.

cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
@verult
Copy link
Collaborator Author

verult commented May 17, 2023

In the latest push:

  • Fixed coverage check
  • Made _from_device_information to (1) discourage end-users from using it, and (2) make it easier to change in the future
  • Renamed _GateRepresentation fields to better reflect their purposes.

@verult verult requested review from maffoo and dstrain115 May 17, 2023 19:26
verult added 3 commits May 17, 2023 21:39
Gateset.gates is a collection of GateFamilies, so most branches in the check don't apply.
@verult verult added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 18, 2023
…factor/griddevice-serialization-by-serializing-gateset
@verult verult merged commit 0066a53 into quantumlib:master May 19, 2023
@verult verult removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants