-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add cirq.TensoredConfusionMatrices
for readout error mitigation.
#4854
Conversation
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.
super cool. I think an abstraction to tensor-product a big cm from small ones is very useful to have.
I strongly worry about using the words and variable names "confusion matrix" to refer both to (ndarray, Sequence[Qid]) tuples and to ReadoutConfusionMatrix as a source of ... confusion
This also uses raw numpy arrays for the "results" bitstrings and an explicit back-mapping to qubit identities; which I think is correct to make this functionality composable. But it puts a big onus on the experimenter like described in #4449 |
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 looks awesome! Few high level questions:
- Should the
measure_confusion_matrix
function have a parallel vs isolated operation mode ? Thinking along the lines of how measure the data for the 2x2 confusion matrices slightly differently for things like p00_isolated vs p00_parallel. WDYT ? - Can we generalize this a little more to make the readout characterization that we also have in the experiments dir obsolete ?
Yes, this is a good idea but I think we can do this in a follow-up PR.
I guess once 1. above is done, we should be able to make the single qubit readout calibration obsolete since it just represents single qubit confusion matrices for each qubit. We will have to go through a deprecation cycle though and I think it's a widely used method so there will be quite a few places to update. I'd recommend to open separate issues for both 1 & 2 above and do them in separate PRs. LMK your thoughts. I've addressed all other comments. cc @mpharrigan |
cirq.ReadoutConfusionMatrix
for readout error mitigation. cirq.TensoredConfusionMatrices
for readout error mitigation.
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 is quite neat implementation!
@mrwojtek Addressed all comments. PTAL. |
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.
Commented on the optimization failure above.
@mrwojtek Fixed. |
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!
Stale review, changes have been addressed.
Automerge cancelled: A required status check is not present. Missing statuses: ['Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage'] |
Fixes #4800.
cc @mrwojtek @mpharrigan