Add boolean support to cirq_google protos #7177
Conversation
dstrain115
requested review from
wcourtney,
vtomole,
verult and
a team
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7177 +/- ##
==========================================
- Coverage 98.13% 98.13% -0.01%
==========================================
Files 1097 1097
Lines 95658 95662 +4
==========================================
+ Hits 93876 93878 +2
- Misses 1782 1784 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -420,6 +420,7 @@ message ArgValue { | |||
| RepeatedDouble double_values = 6; | |||
| RepeatedString string_values = 7; | |||
| tunits.Value value_with_unit = 8; | |||
| bool bool_value = 9; | |||
There was a problem hiding this comment.
nit: can we move this field next to bool_values for better discoverability in the future. (I'm aware this convention hasn't been done before.)
There was a problem hiding this comment.
I don't think that's a good idea. I think it's generally better to keep things in order of field id unless there's a really good reason. Otherwise, it gets more confusing to add fields.
| @@ -128,7 +128,9 @@ def arg_to_proto( | |||
| """ | |||
| msg = v2.program_pb2.Arg() if out is None else out | |||
|
|
|||
| if isinstance(value, FLOAT_TYPES): | |||
| if isinstance(value, bool): | |||
There was a problem hiding this comment.
on line 146, there is a check for np.bool_, should that be included in this check?
* Add boolean support to cirq_google protos * Add np.bool. * Fix type issue.
No description provided.