-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow passing a callable to de/serialization funcs #6855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow passing a callable to de/serialization funcs #6855
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6855 +/- ##
==========================================
- Coverage 97.86% 97.85% -0.01%
==========================================
Files 1084 1084
Lines 93954 94013 +59
==========================================
+ Hits 91944 91997 +53
- Misses 2010 2016 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I am missing something, but I am not sure why is this needed.
The main code seems to be handling Linspace with units just fine,
so why is it necessary to support conversion of plain Linspace to Linspace with-units inside sweep_to_proto
with a special callable?
import cirq
import tunits
from cirq_google.api import v2
sweep = cirq.Linspace('foo', 3 * tunits.ns, 6 * tunits.ns, 3)
print(sweep)
msg = v2.sweep_to_proto(sweep)
sweep_rt = v2.sweep_from_proto(msg)
print(sweep_rt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@senecameeks Great idea Seneca! ... just a few changes
@pavoljuhas this is to allow serialization of sweeps that use the internal units library ... otherwise we will need to replicate the logic in these functions internally.
Echoing what Nour said, this ability is needed to support serialization of the internal unit library @pavoljuhas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
sweep: cirq.Sweep, | ||
*, | ||
out: Optional[run_context_pb2.Sweep] = None, | ||
sweep_transformer: Callable[[cirq.Points | cirq.Linspace], cirq.Sweep] = lambda x: x, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note in cirq, we have cirq.SingleSweep
(the parent class of cirq.Points
and cirq.Linspace
).
So it can be more precise with
single_sweep_transformer: Callable[[cirq.SingleSweep], cirq.SingleSweep]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@senecameeks for typing you can do this
_SWEEP_T = TypeVar('_SWEEP_T', cirq.Points, cirq.Linspace) # or just cirq.SingleSweep
Callable[[_SWEEP_T], _SWEEP_T]
this will remove the cast statements below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still causes mypy to fail, but also I prefer using cirq.SingleSweep
in the call over type vars since IMO the former is more readable. So I'll keep as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the TypeVar tells mypy that the type of the output is the same as the type of the input e.g. if input is Linspace mypy will expect the output to also be Linspace
msg = v2.sweep_to_proto(sweep) | ||
sweep = v2.sweep_from_proto(msg, sweep_transformer=add_tunit_func) | ||
|
||
assert list(sweep.points)[0] == [1.0 * tunits.ns] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add another test that add tunit
in sweep_to_proto
while remove tunit
in sweep_from_proto
, this is a more practical usage in the pyle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@NoureldinYosri why it is Cirq/cirq-google/cirq_google/api/v2/run_context.proto Lines 210 to 215 in 2448121
|
@BichengYing tunits doesn't have a Unit in the public api ... for performance units are implemented in the cython layer as essnetially arrays ... the public api of tunits has only Value, ValueArray and their subclasses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM%typing nit & other reviewers' comments
sweep: cirq.Sweep, | ||
*, | ||
out: Optional[run_context_pb2.Sweep] = None, | ||
sweep_transformer: Callable[[cirq.Points | cirq.Linspace], cirq.Sweep] = lambda x: x, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@senecameeks for typing you can do this
_SWEEP_T = TypeVar('_SWEEP_T', cirq.Points, cirq.Linspace) # or just cirq.SingleSweep
Callable[[_SWEEP_T], _SWEEP_T]
this will remove the cast statements below
* allow passing func to de/serialization funcs * coverage * simplify * typecheck * nit * mypy * comments * comments
This gives us flexibility to manipulate the cirq.Sweep to add units for example.