Skip to content
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

Coherence limit error of gates with three or more qubits #779

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

itoko
Copy link
Contributor

@itoko itoko commented Apr 22, 2022

Summary

Extend coherence limit error computation to handle gates with three or more qubits.

Details and comments

Deprecate the original function RBUtils.coherence_limit and reimplement it from scratch as a new function RBUtils.coherence_limit_error.

@itoko
Copy link
Contributor Author

itoko commented Apr 25, 2022

Is there a better place to put the new function? Should it be a top-level function rather than a static method under RBUtils?

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review!

@@ -199,6 +206,83 @@ def coherence_limit(nQ=2, T1_list=None, T2_list=None, gatelen=0.1):

return coherence_limit_err

@staticmethod
def coherence_limit_error(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are going to deprecate RBUtils class since this just behaves as a scope. I think this method should be placed in somewhere else. Although I think this is not limited to RB, it would be great if this value can be automatically computed for RB based on given backend property or instmap, and populate the metadata of EPG entries.

Currently we don't have any utils or tool collection in experiments. Perhaps it's good time to introduce something like qiskit_experiments.utils? @chriseclectic

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think stand alone functions like this should be added to qiskit experiments at all unless they are used directly in an experiment or its analysis, and for those cases they should typically be an internal method of those classes (this also applies to all the RBUtils functions that weren't actually being used as part of RB experiment/analysis)

If this is intended to be used as part of RB experiment to construct an analysis result, it should be integrated into RB. Otherwise I don't think really think it belongs in this repo and we need to think of a better home for these sort of utility functions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We often compute this value to investigate the error budget. It might be good to compute the error limit with this function as a metadata of EPG entries (as IRB compute theoretical error bounds). But this function can be used more widely to discuss lower error bound. In that sense perhaps this can be move to terra?

raise ValueError("Length of t1s/t2s must equal num_qubits")

def amplitude_damping_choi(t1, t2, time):
return qi.Choi(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary a local function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No necessary. It's just for readability.

itoko and others added 2 commits April 27, 2022 09:05
Co-authored-by: Naoki Kanazawa <[email protected]>
@itoko itoko force-pushed the enhance-coherence-limit branch from 51beaf8 to 73488cd Compare April 27, 2022 00:06
@ShellyGarion
Copy link
Contributor

Does this PR also handle issue #133 ?

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2023

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants