-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add json serialization to sweeps #5618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (% one concern, below). What is the expected use case for this?
@@ -196,6 +197,9 @@ def param_tuples(self) -> Iterator[Params]: | |||
def __repr__(self) -> str: | |||
return 'cirq.UnitSweep' | |||
|
|||
def _json_dict_(self) -> Dict[str, Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the name, _Unit
is a private class. Do we want it to be serializable?
If so, I think this needs a _from_json_dict_
classmethod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up being necessary in order to serialize UnitSweep
. This is similar, I think to the Pauli singletons. My criteria was that with this PR it should serialize and deserialize cirq.UnitSweep
correctly (should be UNIT_SWEEP
btw), which it does. I think you can also use it to serialize and deserialize _Unit
by going in and grabbing that class explicitly, but I don't know a way to stop this for this singleton pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. This can merge once the tests are sorted out.
Use case is that objects that set our context for experiments should be serializable. |
Ugh bad merge fixing |
Automerge cancelled: A required status check is not present. Missing statuses: ['Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage'] |
Automerge cancelled: A required status check is not present. Missing statuses: ['Coverage check', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage'] |
Adds missing json serialization for sweep objects.
Adds missing json serialization for sweep objects.
Adds missing json serialization for sweep objects.