Cleanup RB module #762

nkanazawa1989 · 2022-04-04T21:23:13Z

Summary

The computation of EPGs from EPC in analysis class introduces heavy dependency on backend and circuit metadata that causes problem for simplification of curve analysis, see #737 for details.

This mainly compute two quantities in the post-process.

gate_error_ratio
This is the ratio of errors wrt the gate kind. In principle, EPC is a single value thus we can extract only single kind of EPG from experiment. There is some trick to compute multiple values, by assuming some dependency among parameters. This is gate_error_ratio. In current implementation the default value is None and it tries to extract the ratio from backend gate properties by directly dividing gate error values, thus it introduces the backend dependency. However, this is not correct (or at least not general) approach, indeed this is based on pretty IBM-ish convention. Originally, this is from convention how we compute U1, U2, U3 error from single EPC value. Here, U1 is only VZ instruction thus error-free. U2 and U3 consist of SY and two SXs. Since SY and SX are identical physical operation (up to mixer skew), we set EPG_U2:EPG_U3 = 1:2. Same thing occurs here, i.e. now we have Rz, SX, X and EPG_Rz:EPG_SX:EPG_X = 0:1:1. Current IBM backends doesn't report exact, i.e. interleaved, EPG for SX and X thus this assumption holds. However, once it reports interleaved EPGs, EPG_SX:EPG_X != 1:1 and the ratio will change from time to time due to fitting error or amplitude nonlinearity (usually ignorable). This example indicates this approach should be removed, not only from viewpoint of cleanup, but also validity. We should always explicitly provide assumption for the gate error, and experiment guarantee the same gates are used in the transpiled circuit.
gate_per_clifford
This is the averaged gate count per single Clifford gate. The gate count is stored in the circuit metadata in the form of tuple(counts, xval). In the post-process gate_per_clifford is inefficiently computed by parsing this metadata just to divide counts by total xval (i.e. total Clifford gates in per-seed experiments). Since experiment knows this xval, it can compute averaged count value before storing it in the circuit metadata. This allows us to get rid of raw_data dependency in the curve analysis post-processing. Indeed this is the only experiment analysis using the raw_data to access circuit metadata (in the formatted data metadata is removed because metadata cannot be averaged over the same x values).

Details and comments

TODO

update unittests, especially for backward compatibility
write releasenote

- remove metadata and backend data from curve analysis instance - update fit figure creation workflow in curve analysis

…nup/rb_remove_rawdata_dependency

nkanazawa1989 · 2022-04-05T19:04:57Z

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

+                gate_per_clifford[(qubits, instr)] += count / total_cliffs
+
+        # Directly update analysis option
+        self.analysis.set_options(gate_per_clifford=gate_per_clifford)


Can we upgrade base experiments so that this value can be passed to experiment data metadata? Otherwise EPGs will disappear in analysis results when we rerun analysis with retrieved experiment data.

This is resolved by swapping the order of transpile and creation of experiment data in the base experiment.

chriseclectic

Initial review. This looks like a good start to cleaning up the RB analysis

qiskit_experiments/library/randomized_benchmarking/interleaved_rb_experiment.py

qiskit_experiments/library/randomized_benchmarking/rb_analysis.py

chriseclectic · 2022-04-05T21:02:46Z

qiskit_experiments/library/randomized_benchmarking/rb_analysis.py

+            gate_error_ratio = experiment_data.metadata.get("gate_error_ratio", None)
+
+            if gate_error_ratio is None:
+                # For backward compatibility when loading old experiment data.


Should we raise the deprecation warning here so its clearer in the code that this is a deprecated path?

Good question. As I wrote in the comment, calling RBUtils.get_error_dict_from_backend raises deprecation warning. I agree writing explicit warning here helps maintainer, but this means user will see the warning twice.

I think here we need to and then add the gnerated gate_error_ratio dict to the experiment data metadata so it can be re-saved to update the saved data, and then add the user/log/deprecation warning that old data was converted to new format and should be resaved to avoid it not working in the future.

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

chriseclectic · 2022-04-05T21:25:47Z

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

+                gate_counts_per_clifford[(qubits, instr)] += count / total_cliffs
+
+        # Directly copy the value to experiment data metadata via instance state
+        self._gate_counts_per_clifford = dict(gate_counts_per_clifford)


Same here, this should update the option value rather than a separate variable

This value is not configurable so having it in options is bit strange. If user provides this value, they should inject already transpiled circuit and avoid transpiring.

My main concern here is that we are storing a temporary variable that depends on specific circuits generated when calling circuits() in the state of the experiment. It seems like an awkward way to get this into metadata.

If we go back to the old way of storing the op_count in circuit metadata, than this computation could be done during analysis which seems to be more where it belongs.

Similar stuff also appears in CR Ham tomo experiments based on our discussion so I don't think this is awkward.
https://github.com/Qiskit/qiskit-experiments/blob/dda5ebe4438b8d0e53f2b0ddda7f6b9dab0c29f3/qiskit_experiments/library/characterization/cr_hamiltonian.py#L173-L175

Calling circuits() doesn't populate this value. This only updated with transpile which is not exposed to users. Anyways I'll revert this logic at cost of double loop.

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

chriseclectic · 2022-04-05T21:26:28Z

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

@@ -242,4 +301,7 @@ def _metadata(self):
        for run_opt in ["meas_level", "meas_return"]:
            if hasattr(self.run_options, run_opt):
                metadata[run_opt] = getattr(self.run_options, run_opt)
+
+        metadata["gate_error_ratio"] = self._gate_error_ratio
+        metadata["gate_counts_per_clifford"] = self._gate_counts_per_clifford


Suggested change

metadata["gate_counts_per_clifford"] = self._gate_counts_per_clifford

metadata["gate_counts_per_clifford"] = self.experiment_options.gate_counts_per_clifford

chriseclectic · 2022-04-06T20:59:47Z

qiskit_experiments/library/randomized_benchmarking/rb_analysis.py

+        if self.options.gate_error_ratio is None:
+            gate_error_ratio = experiment_data.metadata.get("gate_error_ratio", None)
+
+            try:


Is this just to handle the case of None? If so you can just do this with an else in the next clause:

if gate_error_ration is None: # For backward compatibility when loading old experiment data. ... else: gate_error_ration = dict(gate_error_ration)

No, it can be False if experiment doesn't want to perform EPG calculation.

qiskit_experiments/library/randomized_benchmarking/rb_analysis.py

chriseclectic · 2022-04-06T21:05:56Z

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

+                gate_counts_per_clifford[(qubits, instr)] += count / total_cliffs
+
+        # Directly copy the value to experiment data metadata via instance state
+        self._gate_counts_per_clifford = dict(gate_counts_per_clifford)


My main concern here is that we are storing a temporary variable that depends on specific circuits generated when calling circuits() in the state of the experiment. It seems like an awkward way to get this into metadata.

If we go back to the old way of storing the op_count in circuit metadata, than this computation could be done during analysis which seems to be more where it belongs.

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

chriseclectic · 2022-04-06T21:13:38Z

qiskit_experiments/library/randomized_benchmarking/rb_utils.py

@@ -294,3 +303,84 @@ def calculate_2q_epg(
            out[qubit_pair] = {gate_2_qubit_type: 3 / 4 * (1 - alpha_c_2q) / n_gate_2q}

        return out
+
+
+def lookup_epg_ratio(gate: str, n_qubits: int) -> Union[None, int]:


You might as well make this a static method of RBUtils so you dont need to import two things. That said we could just remove the RBUtils class since its basically just acting as a scope for a bunch of static methods

Yes I think this doesn't need to be a class. We should convert these methods into functions, but this can be done in follow-up (if you want I'll do this now)

qiskit_experiments/library/randomized_benchmarking/rb_utils.py

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

Co-authored-by: Christopher J. Wood <[email protected]>

chriseclectic

Reviewing this again I am now leaning towards making InterleaveRBAnalysis a direct subclass of CurveAnalysis. Since it seems we are doing a lot of special case handling in the options of RBAnalysis to allow for differences in its subclass, which is probably not the best way to do this.

If you have methods you want to use in both classes, you could just make them functions like you have with _lookup_epg_ratio that can be imported for both classes, rather than using inherited methods.

qiskit_experiments/library/randomized_benchmarking/rb_analysis.py

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

releasenotes/notes/cleanup-rb-experiment-f17b6e674ae4e473.yaml

qiskit_experiments/library/randomized_benchmarking/rb_analysis.py

chriseclectic · 2022-04-20T13:16:07Z

qiskit_experiments/library/randomized_benchmarking/rb_analysis.py

+        self, experiment_data: ExperimentData
+    ) -> Tuple[List[AnalysisResultData], List["pyplot.Figure"]]:
+
+        if self.options.gate_error_ratio is not False:


I'm still not thrilled about having different behavior of None vs False for this option value. It seems a lot of this complication is because of using RBAnalysis as a base class for IRB even though they don't fit super well, so I supposed it would it be better to just change InterleavedRBAnalysis to be a direct subclass of CurveAnalysis instead of RBAnalysis

Done in 357f0a4

nkanazawa1989 · 2022-04-20T13:33:22Z

docs/tutorials/randomized_benchmarking.rst


-    # Compare the computed EPG of the cx gate with the backend's recorded cx gate error:


This comparison is removed because it doesn't make sense. In IBM backend conventionally 2Q gate EPG is computed by just dividing EPC by 1.5.

chriseclectic

This looks much cleaner with the direct analysis subclass. I left a couple of minor suggestions.

It might be good to get someone else to proof read the RB tutorial, but any changes to that could be made into a follow up PR

qiskit_experiments/curve_analysis/guess.py

chriseclectic · 2022-04-20T20:58:35Z

qiskit_experiments/library/randomized_benchmarking/rb_analysis.py

+            gate_error_ratio (Optional[Dict[str, float]]): A dictionary with gate name keys
+                and error ratio values used when calculating EPG from the estimated EPC.
+                The default value will use standard gate error ratios.
+                If set to ``False`` EPG will not be calculated.


Do we still need the False case?

Depends if you want to avoid computation of EPG. If you don't trust gate error ratio then you should be able to disable this to avoid creating some garbage in the database.

In that case I think it should be None instead of false for the skip case (since you now use "default" for computing defualt)

Co-authored-by: Christopher J. Wood <[email protected]>

…nazawa1989/qiskit-experiments into cleanup/rb_remove_rawdata_dependency

nkanazawa1989 · 2022-04-21T02:13:01Z

Thanks Chris. I'll merge this to move forward. Other people can update tutorial before the release if they want.

chriseclectic · 2022-04-21T12:58:22Z

test/randomized_benchmarking/test_randomized_benchmarking.py

@@ -350,6 +346,22 @@ def test_non_clifford_interleaved_element(self):
            lengths=lengths,
        )

+    def test_interleaving_delay(self):


You should skip this test until the 0.20.1 terra patch is released which includes the required fix there.

requirements.txt

* - remove backend and raw data dependency from RB * fix qiskit-community#727 Co-authored-by: Christopher J. Wood <[email protected]>

nkanazawa1989 added 3 commits April 5, 2022 05:49

- remove backend and raw data dependency from RB

833621e

- remove metadata and backend data from curve analysis instance - update fit figure creation workflow in curve analysis

Merge branch 'main' of github.com:Qiskit/qiskit-experiments into clea…

f214707

…nup/rb_remove_rawdata_dependency

deprecate public methods

0350cef

nkanazawa1989 requested a review from chriseclectic April 4, 2022 21:23

nkanazawa1989 changed the title ~~Cleanup standard RB EPG computation~~ [WIP] Cleanup standard RB EPG computation Apr 4, 2022

nkanazawa1989 force-pushed the cleanup/rb_remove_rawdata_dependency branch from 005578c to 115d4ef Compare April 5, 2022 19:00

nkanazawa1989 commented Apr 5, 2022

View reviewed changes

update handling of gate_error_ratio

8acb0be

nkanazawa1989 force-pushed the cleanup/rb_remove_rawdata_dependency branch from 4c69819 to 8acb0be Compare April 5, 2022 19:49

code formatting

7785cff

nkanazawa1989 force-pushed the cleanup/rb_remove_rawdata_dependency branch from a48cfad to 7785cff Compare April 5, 2022 20:05

minor bug fix

72641ac

chriseclectic reviewed Apr 5, 2022

View reviewed changes

nkanazawa1989 added 8 commits April 6, 2022 16:36

create gate error ratio for all basis gates

8db7f10

replace "skip" with False

e98e42a

fix json serialization of metadata

a3540b3

fix gate counting bug

ae71972

lint

15187b7

add more test

533a0c5

add reno

b7c0e94

lint

e9c66b5

nkanazawa1989 force-pushed the cleanup/rb_remove_rawdata_dependency branch from ae868fe to e9c66b5 Compare April 6, 2022 15:32

nkanazawa1989 changed the title ~~[WIP] Cleanup standard RB EPG computation~~ Cleanup standard RB EPG computation Apr 6, 2022

remove redundant internal state for gate error ratio

e53521b

nkanazawa1989 added this to the Release 0.3 milestone Apr 6, 2022

This was referenced Apr 6, 2022

CurveAnalysis refactoring #737

Closed

CurveAnalysis base class #765

Merged

chriseclectic reviewed Apr 6, 2022

View reviewed changes

nkanazawa1989 added 2 commits April 9, 2022 02:11

upgrade EPG computation

50e1269

replace warning with logging

d55185a

nkanazawa1989 and others added 2 commits April 20, 2022 19:51

fix doc of gate_error_ratio

676047b

Co-authored-by: Christopher J. Wood <[email protected]>

update reno

de07ff4

chriseclectic reviewed Apr 20, 2022

View reviewed changes

update documentation and move API docs to tutorial

3ab25f5

nkanazawa1989 commented Apr 20, 2022

View reviewed changes

nkanazawa1989 added 4 commits April 20, 2022 22:38

move helper function after the class

4fdc2b3

remove gate counts from analysis options

5f47780

replace baseclass of IRB analysis.

357f0a4

cleanup tutorial

af231ec

nkanazawa1989 changed the title ~~Cleanup standard RB EPG computation~~ Cleanup RB module Apr 20, 2022

fix qiskit-community#727

c9becae

nkanazawa1989 force-pushed the cleanup/rb_remove_rawdata_dependency branch from c4bc7af to c9becae Compare April 20, 2022 20:36

chriseclectic approved these changes Apr 20, 2022

View reviewed changes

nkanazawa1989 and others added 4 commits April 21, 2022 09:58

Update qiskit_experiments/curve_analysis/guess.py

3a9c21a

Co-authored-by: Christopher J. Wood <[email protected]>

lint

71df88c

Merge branch 'cleanup/rb_remove_rawdata_dependency' of github.com:nka…

2b142df

…nazawa1989/qiskit-experiments into cleanup/rb_remove_rawdata_dependency

Merge branch 'main' into cleanup/rb_remove_rawdata_dependency

e246f71

chriseclectic requested changes Apr 21, 2022

View reviewed changes

nkanazawa1989 force-pushed the cleanup/rb_remove_rawdata_dependency branch from c51df80 to 4cd5b8f Compare April 21, 2022 13:12

fix bug

6197b3c

nkanazawa1989 force-pushed the cleanup/rb_remove_rawdata_dependency branch from 4cd5b8f to 6197b3c Compare April 21, 2022 13:48

chriseclectic approved these changes Apr 21, 2022

View reviewed changes

chriseclectic merged commit 8628f23 into qiskit-community:main Apr 21, 2022

nkanazawa1989 deleted the cleanup/rb_remove_rawdata_dependency branch April 21, 2022 19:16

chriseclectic added Changelog: API Change Include in the "Changed" section of the changelog Changelog: Deprecation Include in "Deprecated" section of changelog labels Apr 25, 2022

nkanazawa1989 mentioned this pull request Apr 25, 2022

Delay support in Interleaved Randomized Benchmarking #776

Merged

itoko mentioned this pull request Apr 26, 2022

InterleavedRB experiment does not support delay instruction #727

Closed

paco-ri pushed a commit to paco-ri/qiskit-experiments that referenced this pull request Jul 11, 2022

Cleanup RB module (qiskit-community#762)

194736d

* - remove backend and raw data dependency from RB * fix qiskit-community#727 Co-authored-by: Christopher J. Wood <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup RB module #762

Cleanup RB module #762

nkanazawa1989 commented Apr 4, 2022 •

edited

Loading

nkanazawa1989 Apr 5, 2022

nkanazawa1989 Apr 5, 2022

chriseclectic left a comment

chriseclectic Apr 5, 2022

nkanazawa1989 Apr 6, 2022

chriseclectic Apr 19, 2022

chriseclectic Apr 5, 2022

nkanazawa1989 Apr 6, 2022

chriseclectic Apr 6, 2022

nkanazawa1989 Apr 7, 2022

chriseclectic Apr 5, 2022

chriseclectic Apr 6, 2022

nkanazawa1989 Apr 7, 2022

chriseclectic Apr 6, 2022

chriseclectic Apr 6, 2022 •

edited

Loading

nkanazawa1989 Apr 7, 2022

chriseclectic left a comment

chriseclectic Apr 20, 2022

nkanazawa1989 Apr 20, 2022

nkanazawa1989 Apr 20, 2022

chriseclectic left a comment

chriseclectic Apr 20, 2022

nkanazawa1989 Apr 21, 2022

chriseclectic Apr 21, 2022

nkanazawa1989 commented Apr 21, 2022

chriseclectic Apr 21, 2022

	metadata["gate_counts_per_clifford"] = self._gate_counts_per_clifford
	metadata["gate_counts_per_clifford"] = self.experiment_options.gate_counts_per_clifford


		# Compare the computed EPG of the cx gate with the backend's recorded cx gate error:

Cleanup RB module #762

Cleanup RB module #762

Conversation

nkanazawa1989 commented Apr 4, 2022 • edited Loading

Summary

Details and comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseclectic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseclectic Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseclectic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseclectic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 commented Apr 21, 2022

Choose a reason for hiding this comment

nkanazawa1989 commented Apr 4, 2022 •

edited

Loading

chriseclectic Apr 6, 2022 •

edited

Loading