-
Notifications
You must be signed in to change notification settings - Fork 131
RB utils class #86
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
RB utils class #86
Conversation
| from qiskit.providers.backend import Backend | ||
|
|
||
|
|
||
| class RBUtils: |
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.
Why this need to be a class? Looks like this is collection of functions.
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.
We might change it since it may be better to put those functions inside of the experiment/analysis classes. So far I'm trying to get the Ignis format to work before committing to more structural changes.
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.
You mean adding them to RBAnalysis as methods? I think this means all RB subclasses inherit those functionality, even though they don't use, i.e. IRB doesn't need to calculate EPG because we can directly estimate it.
ShellyGarion
left a comment
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.
Generally, the code LGTM! But it's difficult to verify that the calculations are correct.
I would suggest to add some examples of EPG and coherence limit calculations to the tutorial like it's done in:
https://qiskit.org/documentation/tutorials/noise/4_randomized_benchmarking.html
nkanazawa1989
left a comment
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.
Hi Gadi, thanks for adding the utils. I'm happy to see we can reuse resources from Ignis. I added some suggestions for handling of options.
| """ | ||
| self._analysis_options.update_options(**fields) | ||
|
|
||
| def _postprocess_transpiled_circuits(self, circuits: List[QuantumCircuit]): |
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.
Post processing is quite limited context and many experiments don't use though we are adding this to the base class. I think returning transpile method back (and calling this method from .run, instead of just calling terra transpiler) would help other experiments too, i.e. in QV experiment people may want override transpiler. @chriseclectic ?
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.
My goal here is to minimize as much as possible taking over processes which the base class should handle. I don't need control over the transpilation stage itself; only a peek into its results (similar to the post processing in use in the analysis class). Of course, I'm happy to hear any suggestion you or @chriseclectic might have.
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 think this sort of step could be useful in general. We might want to add backend and run_options to it since they might be needed for some post-processing (eg for adding measurement level metadata for a calibration experiment)
def _postprocess_transpiled_circuits(self, circuits, backend, **run_options):
"""Additional post-processing of transpiled circuits before running on backend"""
pass| @staticmethod | ||
| def coherence_limit(nQ=2, T1_list=None, T2_list=None, gatelen=0.1): | ||
| """ | ||
| The error per gate (1-average_gate_fidelity) given by the T1,T2 limit. |
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.
Can you write physics behind this? This is quite convenient but I felt some difficulty of using this in journal parpers, because we don't provide any technical reference for this calculation.
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.
Note that this code was copied from the ignis code written by Dave, so you should refer this question to him:
https://github.com/Qiskit/qiskit-ignis/blob/9201ed8f9970c50308601f4dfd895a1b8255e469/qiskit/ignis/verification/randomized_benchmarking/rb_utils.py#L169-#L224
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 not originally my code, but I'll try.
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.
Actually I don't know the original paper of this calculation. Seems like some conventional code used by IBM experimentalists. One thing really close to this form is shown in Appendix G of this paper.
It would be great if you can derive the coherence limit, but this is not mandatory for this PR. Can you write an issue for future extension?
| found_gates.append(gate) | ||
| error_sum += gates_per_clifford[key] * value | ||
| for gate in found_gates: | ||
| epg[qubit][gate] = (error_dict[((qubit,), gate)] * epc_1_qubit) / error_sum |
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.
It would be better to keep sigma.
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.
Not sure I follow you - sigma instead of what?
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.
Tough EPC has error (sigma=stdev), this information is not used by EPG. Perhaps this is future extension. Can you write an issue for reminder?
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.
Since EPG is calculated from EPC, it should be possible to calculate the error bounds for EPG.
nkanazawa1989
left a comment
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.
Thanks Gadi, the structure looks good. There are some nitpicks and test idea. Let's hear opinion of @chriseclectic about base class change.
| """A collection of utility functions for computing additional data | ||
| from randomized benchmarking experiments""" | ||
|
|
||
| @staticmethod |
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.
Perhaps you want to decorate with @classmethod? Same for others. (i.e. there is no code creating an instance of RBUtil)
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 what @staticmethod does. In @classmethod the class itself is a parameter (for setting class variables etc.) and I don't need it here.
* Added the rb_utils file from Ignis as a class * Adding transpiled circuits postprocessing * Computing gate count by qubit * gates_per_clifford was simplified and removed from rb_utils * Getting the error dict from the backend * Computing gates per clifford * EPG for 1 qubit is now working * Refactoring for EPG on 2 qubits * 1 qubit epg is working after refactoring * 2 qubit epg is done but might have bugs * Finalizing epg analysis and removing debug prints * Added documentation * Linting * Small fix * Integrating the curve analysis class with the epg computation in RB * Added epg computation test * Bugfix in n_gate_2q computation * Passing RB analysis data via options * Linting * Linting * Linting * Lower test sensitivity * Adding more lengths to the rb test * Linting * Temporarily disabling test due to nondeterministic behavior on testing machine * Temporarily disabling test due to nondeterministic behavior on testing machine * Gates per clifford is now computed in the analysis section * gate_error_ratio is now an analysis option and instead of passing backend, we pass error dict * Linting * Small fix * Small fixes * Added count_ops test * Added calculate_1q_epg test * Linting * Linting * Moving error_dict computation from experiment to analysis
Summary
Details and comments