Make the points in cirq_google v2 proto to be double#7498
Make the points in cirq_google v2 proto to be double#7498BichengYing merged 11 commits intoquantumlib:mainfrom
Conversation
| @@ -228,6 +228,10 @@ message Points { | |||
| // The values. | |||
| repeated float points = 1; | |||
There was a problem hiding this comment.
Consider marking the old points with [deprecated = true] and adding a comment that the point_list should be used instead.
There was a problem hiding this comment.
Yes, this is a good practice but we cannot add it [deprecated = true] for now (avoid unnecessary warning) because before we roll-out the latest version into the servers of all machines, this field is still used. I will add a comment first.
There was a problem hiding this comment.
IIUC, deprecated fields are usable just as before except they might show deprecation warnings when used. Such warnings can be helpful to identify client code that uses the old fields.
There was a problem hiding this comment.
But because of current dual writting mechanism, this warning will be popped out always while the user has no action to do when saw this warning. I feel this will confuse the external users. So I still think this [deprecated = true] should be added after all servers are updated and we stopped the dual write mechanism .
|
|
||
| // Due to historical reason, the points are stored as float32. | ||
| // But float64 should be a better choice. | ||
| repeated double point_list = 3; |
There was a problem hiding this comment.
I think we also need to extend Linspace and ConstValue to make sure they're consistent.(http://shortn/_TLCXefjbb8)
There was a problem hiding this comment.
Ok, done. Note ConstValue used oneof, hence, the dual-write trick cannot work here. That one will be switched at last
|
Also, fyi, to make the naming consistently, I use |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7498 +/- ##
=======================================
Coverage 98.68% 98.68%
=======================================
Files 1091 1091
Lines 96853 96884 +31
=======================================
+ Hits 95579 95610 +31
Misses 1274 1274 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pavoljuhas
left a comment
There was a problem hiding this comment.
LGTM with some small suggestions.
|
|
||
| // Due to historical reason, the points are stored as float32. | ||
| // But float64 should be a better choice. | ||
| repeated double points_fl64 = 3; |
There was a problem hiding this comment.
Nit - consider renaming to points_double here and similarly in Linspace for consistency with ConstValue.double_value.
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
In order to make it forward and backward compatible, i added an extra field
points_fl64andfloat pointsanddouble points_fl64points_fl64first if it presented, if not, fall back topoints.It will temporary make the communication cost slightly higher since we communicating two points. But it will be make the transition much easier.
UPDATE: added the double field for LinSpace and Const as well.