Use v2 protos in cirq.google.Engine #1779
Conversation
A default config can be generated from the circuit using
cirq.google.api.v2.find_measurments.
| def results_from_proto( | ||
| msg: result_pb2.Result, | ||
| measurements: List[MeasureInfo], | ||
| measurements: List[MeasureInfo] = None, |
There was a problem hiding this comment.
I think instead of making this optional and relying on ordering, we should use the api_v2.find_measurements function to find measurements in the program before sending to the quantum engine, and store this in the EngineJob. Then when getting results the list of measurements can be passed in to results_from_proto.
There was a problem hiding this comment.
Shouldn't all the info about the measurements be accessible from the results?
There was a problem hiding this comment.
What is your concern with relying on the order in the result? If we want to support re-ordering, could it be a feature/utility rather than a requirement to parse a result?
There was a problem hiding this comment.
We want to be sure that the order in which results appear in TrialResult match the original measurement operation. The measurements arg on results_from_proto is here so we can check the qubit order in the results against the original measure ops, and it's as easy to reorder as to check so that's what it does. If we want to specify that the engine will not reorder (and then assume but not check this), then I'd suggest we remove this argument entirely rather than making it optional.
I think it would be good to include the qubit order in TrialResult itself to make the results more self-describing. That would alleviate some of my concern because we won't have so many assumptions about parallel ordering between various objects. I know TrialResult is getting an overhaul, so maybe we can include the qubit order as part of that change.
There was a problem hiding this comment.
+1 to including qubit order details in the result.
I'd prefer to remove the measurements arg, but that would be a breaking change for existing external dependencies (I found one). My plan was to make this optional to continue support, then, if it was acceptable, remove all references, then clean up by removing the param. LMK if it's fine to just make the breaking change, otherwise I can add comments to that effect.
There was a problem hiding this comment.
I agree with @dabacon, making this optional for now is fine. We can remove in a future PR.
| from cirq.google.convert_to_xmon_gates import ConvertToXmonGates | ||
| from cirq.google.params import sweep_to_proto_dict | ||
| from cirq.google.programs import schedule_to_proto_dicts, unpack_results | ||
| from cirq.google.serializable_gate_set import SerializableGateSet | ||
| from cirq.schedules import Schedule, moment_by_moment_schedule | ||
| from cirq.study import ParamResolver, Sweep, Sweepable, TrialResult | ||
| from cirq.study.sweeps import Points, UnitSweep, Zip | ||
|
|
There was a problem hiding this comment.
nit: this follows a very different import strategy than we follow in Cirq. No need to fix now, but generally we don't import classes / method
cirq/google/engine/engine.py
Outdated
| program_dict['operations'] = [ | ||
| op for op in schedule_to_proto_dicts(schedule) | ||
| ] | ||
| return program_dict, None # run context included in program |
There was a problem hiding this comment.
is there a reason to do this for v1 protos? It would be nice to always just the the run context.
There was a problem hiding this comment.
We previously talked about making these changes separately, so I left it as-is. It should be straightforward, so I'll address this after attending to the other comments.
| def results_from_proto( | ||
| msg: result_pb2.Result, | ||
| measurements: List[MeasureInfo], | ||
| measurements: List[MeasureInfo] = None, |
There was a problem hiding this comment.
Shouldn't all the info about the measurements be accessible from the results?
Strilanc
changed the title
…what is used to encode a program on the client.
…refactoring of existing code
Merge branch 'master' into engine-v2-protos
|
I'm fine with the None if we eventually migrate it away in external code. @maffoo ? |
| def results_from_proto( | ||
| msg: result_pb2.Result, | ||
| measurements: List[MeasureInfo], | ||
| measurements: List[MeasureInfo] = None, |
There was a problem hiding this comment.
I agree with @dabacon, making this optional for now is fine. We can remove in a future PR.
Update cirq Engine to optionally use new proto definitions in cirq.api.google.v2 when communicating with the API.