-
Notifications
You must be signed in to change notification settings - Fork 985
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 WaitGate proto serialization #2437
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.
Can server side support this gate?
cirq/google/common_serializers.py
Outdated
@@ -280,3 +280,24 @@ def _near_mod_2(e, t, atol=1e-8): | |||
gate_constructor=lambda: ops.FSimGate(theta=-np.pi / 4, phi=0), | |||
args=[]), | |||
] | |||
"""Measurement serializer.""" |
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.
Fix comments. Also, Matthew suggested in a different PR that we use comments instead of string literals to improve formatting (I'll fix the other ones)
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.
Done.
cirq/google/gate_sets.py
Outdated
SINGLE_QUBIT_HALF_PI_SERIALIZERS, SINGLE_QUBIT_HALF_PI_DESERIALIZERS, | ||
MEASUREMENT_SERIALIZER, MEASUREMENT_DESERIALIZER, SYC_SERIALIZER, | ||
SYC_DESERIALIZER, SQRT_ISWAP_SERIALIZERS, SQRT_ISWAP_DESERIALIZERS, | ||
WAIT_GATE_SERIALIZER, WAIT_GATE_DESERIALIZER) |
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.
Add trailing comma so that the formatting remains the same?
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.
Done.
cirq/google/gate_sets.py
Outdated
@@ -36,12 +30,14 @@ | |||
*SINGLE_QUBIT_SERIALIZERS, | |||
*SINGLE_QUBIT_HALF_PI_SERIALIZERS, | |||
MEASUREMENT_SERIALIZER, | |||
WAIT_GATE_SERIALIZER, |
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.
We should not add this to the gate set until it is supported at the server
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.
i.e. add the serializer first but not to the gate set.
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.
Isn't the server using this same gate set object, though? How can I add it to one before the other if they're the same thing?
I suppose I could add it to the deserializers first.
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.
Done.
Refactored this to only define the serializer and deserializer, without adding it to the gate sets. |
op_serializer.SerializingArg( | ||
serialized_name='nanos', | ||
serialized_type=float, | ||
gate_getter=lambda e: e.duration.total_nanos()), |
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.
I wonder if we should put a limit here, like don't allow waits of more than 10 milliseconds (or should this be on the pyle side)?
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.
Yeah, that'd be a good idea.
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.
At least maybe yell if this is negative for now.
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, besides one concern (that may not belong in this PR anyway)
No description provided.