Add serialization of tokens #3480
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Adds tokens as a seperate proto field. - Adds a constant table for lookup of tokens. - Constant is a symbol table where we can store constant strings. It is its own message, so it could potentially be used for other objects (such as gates) in the future, but for now, it is only used to store tokens. Note: symbol is already used for sympy symbols, so used the phrase Constant instead.
maffoo
reviewed
Nov 4, 2020
cirq/google/api/v2/program.proto
Outdated
| // | ||
| // The token can be specified as a string or as a reference to | ||
| // the constant table of the circuit. | ||
| oneof arg { |
There was a problem hiding this comment.
Maybe call the overall oneof field "token" instead? Also might be nice to use a common prefix on the fields to make clear they relate to the token:
oneof token {
string token_value = 4;
int32 token_constant_index = 5;
}
cirq/google/op_deserializer.py
Outdated
| return self.op_wrapper(gate.on(*qubits), proto) | ||
| op = self.op_wrapper(gate.on(*qubits), proto) | ||
| if self.auto_deserialize_tokens: | ||
| if proto.HasField('constant_index') and constants: |
There was a problem hiding this comment.
I would suggest checking which case is populated with WhichOneof:
which = proto.WhichOneof('token')
if which == 'token_constant_index':
...
elif which == 'token_value':
...Also, if a constant index is specified but constants is empty, we should probably raise an error.
cirq/google/op_serializer.py
Outdated
| @@ -68,7 +71,8 @@ def __init__(self, | |||
| serialized_gate_id: str, | |||
| args: List[SerializingArg], | |||
| can_serialize_predicate: Callable[['cirq.Operation'], bool] = | |||
| lambda x: True): | |||
| lambda x: True, | |||
| auto_serialize_tokens: Optional[bool] = True): | |||
There was a problem hiding this comment.
nit: It seems to me "auto" does not add much here; could we call this just serialize_tokens? Similarly for the deserializer above, could rename auto_deserialize_tokens -> deserialize_tokens.
cirq/google/op_serializer.py
Outdated
| # Token not found, add it to the list | ||
| msg.constant_index = len(constants) | ||
| new_constant = v2.program_pb2.Constant() | ||
| new_constant.string_value = tag.token |
There was a problem hiding this comment.
Could reuse the same constant object instead of recreating:
constant = v2.program_pb2.Constant()
constant.string_value = tag.token
try:
msg.constant_index = constants.index(constant)
except ValueError:
msg.constant_index = len(constants)
constants.append(constant)
cirq/google/op_serializer_test.py
Outdated
| GateWithAttribute(0.125)(q).with_tags(tag), constants=constants) | ||
| constant = v2.program_pb2.Constant() | ||
| constant.string_value = 'my_token' | ||
| assert constants == [constant] |
There was a problem hiding this comment.
Would be good to test that if you serialize an op and already have a matching constant it will use the same index.
cirq/google/serializable_gate_set.py
Outdated
| arg_function_language=arg_function_language) | ||
| arg_function_language=arg_function_language, | ||
| constants=constants) | ||
| msg.constants.extend(constants) |
There was a problem hiding this comment.
Should guard here:
if constants is not None:
msg.constants.extend(constants)
This was referenced Nov 4, 2020
…115/Cirq-1 into token_serialization_try2
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is its own message, so it could potentially be used for other
objects (such as gates) in the future, but for now, it is only used
to store tokens.
Note: symbol is already used for sympy symbols, so used the
phrase Constant instead.