Add support for const sweep with None #6729
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6729 +/- ##
=======================================
Coverage 97.83% 97.83%
=======================================
Files 1077 1077
Lines 92485 92524 +39
=======================================
+ Hits 90484 90523 +39
Misses 2001 2001 ☔ View full report in Codecov by Sentry. |
BichengYing
requested review from
wcourtney,
vtomole,
verult and
a team
as code owners
| @@ -63,7 +91,10 @@ def sweep_to_proto( | |||
| out.single_sweep.parameter.units = sweep.metadata.units | |||
| elif isinstance(sweep, cirq.Points) and not isinstance(sweep.key, sympy.Expr): | |||
| out.single_sweep.parameter_key = sweep.key | |||
| out.single_sweep.points.points.extend(sweep.points) | |||
| if len(sweep.points) == 1 and sweep.points[0] is None: | |||
There was a problem hiding this comment.
Should this support arbitrary const values, not just None? If so, can you add a test for the new logic
There was a problem hiding this comment.
Right, it can. But I found other places used sweep with single point as well. So I add this extra condition to avoid the change of previous behavior. (Add the comment in the code base). I think we can remove this is None later when it becomes necessary?
There was a problem hiding this comment.
How many places in cirq use const sweep points? If they're only tests, can we change them?
There was a problem hiding this comment.
Not many. All of them should be under the test of cirq-google only. I have modified them all.(One improvement is we can distinguish float and int now) PTAL.
But I am not sure if Cirq Server in Pyle will be impacted or not. If always using sweep_to_proto and sweep_from_proto in pair, we don't need to worry. Otherwise, we should revert it.
There was a problem hiding this comment.
In cirq_server we always use sweep_from_proto to decode from sweep_to_proto so this should be fine.
|
@BichengYing please add a test for supporting arbitrary values |
Done. Added more tests |
No description provided.