-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Publish sample Z phase errors #5378
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
Conversation
CC @vcatherine |
@@ -37,6 +37,8 @@ | |||
if TYPE_CHECKING: | |||
import cirq | |||
|
|||
_ZPhaseData = Dict[str, Dict[str, Dict[Tuple[ops.Qid, ...], float]]] |
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.
Optional: Do you think it is better to call this _ZPhaseDataType?
Also, it might be good to document what the keys of this dict of dict of dict 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.
When I made this, I didn't realize we already had PhasedFSimCalibrationResult which uses a different structure but fulfills the same purpose. Do you think converting to that would help? (Converting may also be useful for #5397)
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.
I am not sure. That class has some extra stuff like an associated EngineJob and Calibration which may not be relevant. I like the idea of using a typed class rather than a complicated nest of dictionaries though.
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.
Tried making this a class, but it didn't help much (mostly ended up being a wrapper around the same nest of dicts). Opted instead to move the type to util, add a docstring, and share it between the files in this PR.
@@ -156,21 +161,40 @@ def noise_properties_from_calibration( | |||
|
|||
# 5. Extract entangling angle errors. | |||
fsim_errors = {} | |||
gate_zphase_code_pairs: Dict[Type['cirq.Gate'], str] = { |
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.
Should this be a CONSTANT and put at the top?
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 for this and similar constants in the function. (Also made gate_times_ns
an argument to partially address the dangling TODO)
@@ -42,6 +43,8 @@ | |||
'weber': 1635923188204, # 2021-11-03 07:06:28.204 UTC | |||
} | |||
|
|||
ZPHASE_DATA = {'rainbow': 'rainbow_zphase.json', 'weber': 'weber_zphase.json'} |
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.
I am a little concerned about the naming scheme here. Do you think we should put in an extra qualifier, such as 'rainbow_sample_zphase.json' or 'rainbow_average_zphase.json' in case we need to distinguish it from data from a specific day?
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.
Found and added dates for these.
@@ -42,6 +43,8 @@ | |||
'weber': 1635923188204, # 2021-11-03 07:06:28.204 UTC | |||
} | |||
|
|||
ZPHASE_DATA = {'rainbow': 'rainbow_zphase.json', 'weber': 'weber_zphase.json'} | |||
|
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.
Do we need to add these files to a package somewhere so they get packaged up correctly?
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.
These are under cirq/devices/calibrations
, which is already packaged in setup.py.
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.
A few nits, but otherwise seems fine to me.
# TODO: acquire this based on the target device. | ||
# Default map of gates to their durations. | ||
DEFAULT_GATE_NS: Dict[Type['cirq.Gate'], float] = { | ||
ops.ZPowGate: 25.0, |
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.
Is this correct? ZPow should generally be zero unless it is a physical Z.
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.
Yes, this is for physical Z. Virtual Z is handled deeper in the noise models.
This PR adds Z phase angles for sqrt-ISWAP gates and tools for accessing them. Follow-up to quantumlib#5364. Z phase angles are captured in a different calibration process than the other error metrics, and thus require a slightly different pipeline to access them. Key points to note about this data: - This only includes zeta and gamma, as theta and phi already appear in the QCS metrics and chi is not currently captured by any calibration process we use. - Qubit (6, 1) was in a bad state when the Weber data was taken, and thus has no Z phase angles in the sample data. The pipeline will treat this as zeta=gamma=0 for gates touching this qubit.
This PR adds Z phase angles for sqrt-ISWAP gates and tools for accessing them. Follow-up to #5364.
Z phase angles are captured in a different calibration process than the other error metrics, and thus require a slightly different pipeline to access them. Key points to note about this data: