Add protos for Focused Calibration #3222
Conversation
dstrain115
commented
Aug 17, 2020
dstrain115
commented
- This adds protos needed for the future feature of custom calibrations.
- This adds protos needed for the future feature of custom calibrations.
cirq/google/api/v2/calibration.proto
Outdated
| // of a calibration procedure. | ||
| message CalibrationLayer { | ||
| // The type of the calibration procedure to execute. | ||
| // There will be a number of calibration types that can be run, |
There was a problem hiding this comment.
Future tense is confusing here (does this exist or not?), please rephrase to present tense. I think we should define the first set of acceptable values upfront as well - which also could be then an enum instead of string. WDYT?
There was a problem hiding this comment.
Yes, I was going to put this in a cirq python enum. We can point to it once it exists.
Our convention (for instance with gate sets, gate ids, and arg functions) is to use strings rather than enums. The idea is that we then do not need to modify the public proto when we add new calibration routines (or expose them publicly for that matter). This allows for easier augmentation. We may add and subtract calibration procedures (fairly) frequently, just as we occasionally add new gates.
If you want, I can add a TODO to point to the cirq enum once it exists?
cirq/google/api/v2/calibration.proto
Outdated
| // if the calibration requires this. For many calibrations, | ||
| // this will be a single moment representing the layer to | ||
| // optimize for. | ||
| Circuit circuit = 2; |
There was a problem hiding this comment.
Does the SchedulingStrategy influence the calibration? How about other future execution specific metada/strategies? I.e should this be Program instead of Circuit?
There was a problem hiding this comment.
We definitely need a gateset at least. Changed to a Program
cirq/google/api/v2/calibration.proto
Outdated
| // Error codes | ||
| enum CalibrationLayerCode { | ||
|
|
||
| // For backwards compatibility. |
There was a problem hiding this comment.
This is a bit confusing - backwards compatibility with what? This didn't exist before...
IIUC this should be never used, and it is here by convention in case a client doesn't set it?
There was a problem hiding this comment.
go/protodosdonts#do-include-an-unspecified-value-in-an-enum
There was a problem hiding this comment.
Gotcha - that makes sense - I think we can expand on that a bit more in this comment and/or link a publicly available link explaining the reason.
Also from that same paragraph:
All enum values declared under a container message are in the same C++ namespace, so prefix the unspecified value with the enum’s name to avoid compilation errors.
i.e. this should be called CALIBRATION_RESULT_UNKNOWN or if we follow the other enums in our protos it might be CALIBRATION_RESULT_UNSPECIFIED.
Co-authored-by: Balint Pato <balopat@users.noreply.github.com>
Co-authored-by: Balint Pato <balopat@users.noreply.github.com>
Co-authored-by: Balint Pato <balopat@users.noreply.github.com>
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
- This adds protos needed for the future feature of custom calibrations.
- This adds protos needed for the future feature of custom calibrations.
