Support for stimcirq gates and operations in cirq_google protos #7101
Conversation
dstrain115
commented
Feb 26, 2025
•
dstrain115
commented
- Adds special casing for stimcirq gates and operations.
- Note that this only supports gates and operations where the arguments can be serialized.
- Serializing the stimcirq gates uses the json dictionary in order to gather arguments from the operations.
- Tests will only be run if stimcirq is installed
- This also adds stimcirq as a dev dependency so that we can test the interaction.
- (end users will not be affected, just developers)
- Adds special casing for stimcirq gates and operations. - Note that this only supports gates and operations where the arguments can be serialized. - Serializing the stimcirq gates uses the json dictionary in order to gather arguments from the operations. - Tests will only be run if stimcirq is installed (manual use only)
|
Important The terms of service for this installation has not been accepted. Please ask the Organization owners to visit the Gemini Code Assist Admin Console to sign it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7101 +/- ##
=======================================
Coverage 98.16% 98.16%
=======================================
Files 1093 1093
Lines 95420 95456 +36
=======================================
+ Hits 93665 93701 +36
Misses 1755 1755 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dstrain115
requested review from
wcourtney,
vtomole,
verult and
a team
as code owners
| @@ -258,6 +257,23 @@ def _serialize_gate_op( | |||
| arg_func_langs.float_arg_to_proto( | |||
| gate.q1_detune_mhz, out=msg.couplerpulsegate.q1_detune_mhz | |||
| ) | |||
| elif op.__module__.startswith('stimcirq') or ( | |||
There was a problem hiding this comment.
prefer getattr(op, "__module__", "")
There was a problem hiding this comment.
Good idea, done.
| @@ -258,6 +257,23 @@ def _serialize_gate_op( | |||
| arg_func_langs.float_arg_to_proto( | |||
| gate.q1_detune_mhz, out=msg.couplerpulsegate.q1_detune_mhz | |||
| ) | |||
| elif op.__module__.startswith('stimcirq') or ( | |||
| gate is not None and gate.__module__.startswith('stimcirq') | |||
There was a problem hiding this comment.
Can replace this entire line with "getattr(gate, "module", "")" since it handles None values gracefully
There was a problem hiding this comment.
Good idea, done.
| msg.internalgate.num_qubits = len(stimcirq_obj.qubits) | ||
|
|
||
| # Store json_dict objects in gate_args | ||
| for k, v in stimcirq_obj._json_dict_().items(): |
There was a problem hiding this comment.
Instead of relying on the stimcirq object to always have _json_dict_ defined, prefer to use getattr instead. Also from your comment on line 266, it's not clear if stimcirq operations will also always have _json_dict_ defined.
There was a problem hiding this comment.
I am not sure I understand what you are proposing. How do we know which attributes to get from the stimcirq object otherwise? Also, how do we know how to instantiate the appropriate object on deserialization.
I think saying that a stimcirq gate/operation must have a json_dict to be imported/exported by cirq_google protos is probably a reasonable requirement (plus, we control both repos, so we can add it if it's missing).
I did add a value error if this condition is not met though.
| op = arg_func_langs.internal_gate_from_proto(operation_proto.internalgate)(*qubits) | ||
| parsed_as_stimcirq = False | ||
| msg = operation_proto.internalgate | ||
| if msg.module == 'stimcirq': |
There was a problem hiding this comment.
nit: Can stimcirq be defined as a constant and used here and above?
| @@ -670,7 +696,31 @@ def _deserialize_gate_op( | |||
| raise ValueError(f"dimensions {dimensions} for ResetChannel must be an integer!") | |||
| op = cirq.ResetChannel(dimension=dimensions)(*qubits) | |||
| elif which_gate_type == 'internalgate': | |||
| op = arg_func_langs.internal_gate_from_proto(operation_proto.internalgate)(*qubits) | |||
| parsed_as_stimcirq = False | |||
There was a problem hiding this comment.
Can we move this code to a local _deserialize_with_stimcirq_if_installed function which would return Optional[cirq.Operation] - and if None it will fallback to internal_gate_from_proto below?
There was a problem hiding this comment.
After factoring out the import, this code block is pretty short. No one gates have a local function.
Let me know if you still think this is a good idea.
| try: | ||
| import stimcirq |
There was a problem hiding this comment.
If stimcirq is not installed this would search sys.path for every deserialized gate.
Consider moving this to a @functools.cache decorated function which returns stimcirq.JSON_RESOLVERS_DICT if installed or an empty dictionary otherwise.
There was a problem hiding this comment.
I think it's only for operations with a module name of stimcirq, but, still, this is a good idea, and done.
cirq-google/cirq_google/serialization/circuit_serializer_test.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Pavol Juhas <[email protected]>
…in115/Cirq-1 into deserialize_stimcirq_gates
