Add support for const sweep with None in cirq-google/cirq_google/api/v2/run_context.proto#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
a team,
verult,
vtomole and
wcourtney
as code owners
| 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 |
mhucka
changed the title
* Add support for const sweep with None * Fix the test * Fix the test coverage * Fix the test coverage * Address the comment * Use const for all single sweep * Add more test for constant * Fix typecheck * Fix the lint
2 participants
No description provided.