Add corresponding serialization for TwoPulseFSimTag#7971
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7971 +/- ##
=======================================
Coverage 99.62% 99.63%
=======================================
Files 1110 1110
Lines 99647 99662 +15
=======================================
+ Hits 99278 99294 +16
+ Misses 369 368 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BichengYing
requested review from
a team,
hoisinberg,
verult,
vtomole and
wcourtney
as code owners
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
I think we should put translate_via_model and translate_to_two_pulse under a oneof in the proto since only one of those two flags may be set.
Otherwise LGTM.
| @@ -219,6 +219,11 @@ message FSimGate { | |||
| // cirq.FSimGate(...).with_tags(cirq_google.FSimViaModelTag()). | |||
| // This field controls how we translate the gate implementation. | |||
| bool translate_via_model = 3; | |||
|
|
|||
| // If true, this is equivalent to: | |||
| // cirq.FSimGate(...).with_tags(cirq_google.TwoPulseFSimTag()). | |||
| // This field controls how we translate the gate implementation. | |||
| bool translate_to_two_pulse = 4; | |||
There was a problem hiding this comment.
Can we put them under a oneof translation_flag or such?
This would make it explicit in the proto that only one of these can be set.
There was a problem hiding this comment.
I really prefer not to do it because likely it can cause backward compatibility and this code is tied with internal pyle code. I will have headache for the sync between internal and external version for this minor improvement. I think the raise Value check is sufficient (and I don't think anyone will use this way). Does that make sense?
There was a problem hiding this comment.
Discussed offline, it seems backwards compatible. So no such concern. Updated the code to use one-of
| if msg.fsimgate.translate_via_model and msg.fsimgate.translate_to_two_pulse: | ||
| raise ValueError("You cannot add both FSimViaModelTag and TwoPulseFSimTag") |
There was a problem hiding this comment.
Maybe let us have the message in a passive voice as do the other exceptions in the module:
| if msg.fsimgate.translate_via_model and msg.fsimgate.translate_to_two_pulse: | |
| raise ValueError("You cannot add both FSimViaModelTag and TwoPulseFSimTag") | |
| if msg.fsimgate.translate_via_model and msg.fsimgate.translate_to_two_pulse: | |
| raise ValueError("The operation cannot have both FSimViaModelTag and TwoPulseFSimTag") |
BTW, if we do the oneof in proto, the condition at 375 will be never true.
Perhaps we should collect a set of special tags instead and handle it accordingly, e.g.,
translation_tags = {tag for tag in op.tags if isinstance(tag, (FSimViaModelTag, TwoPulseFSimTag))}
if len(translation_tags) > 1: ...
There was a problem hiding this comment.
yeah, this is almost impossible case. With this oneof, I think we can just simply remove it
There was a problem hiding this comment.
I think it is better to keep the check. The op is a cirq object which can have both tags assigned, and then it is unclear what should it be. (The serializer will set the translate_to_two_pulse, but that is only because TwoPulseFSimTag is checked for last)
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
Let us restore back the test_serialize_fsim_both_translation_tags which you had here before. Those 2 tags can be still added together which is ambiguous for serialization and should be disallowed.
Sure. Done |
3 participants
No description provided.