Mitigation experiment

[gadial]

Summary

Details and comments

[ShellyGarion]

• Mitigation experiment should return a Mitigator object and not only the matrices, the matrices are only part of the Mitigator object
• Perhaps there should be only one experiment, and use a method parameter (complete, tensored, CTMP, M3,...)

[eggerdj]

Can we also have a corresponding data processing node? It seems like one would want to use the result from readout error mitigation in a data processing chain.

[chriseclectic]

This review is coming to late since it was prematurely merged, but I strongly suggest you open a new PR to address these comments. While the basic implementation code looks fine this is completely missing API docs. All new experiments added to this module should include detailed API docs so someone reading them can know what an experiment does, how to use it, what its analysis will return, without having to look at the source code, and ideally without previously being familiar with the experiment.

Wrt API docs the name and descriptions of the experiments are incorrect. There is no mitigation being performed by these experiments, since they are characterization experiments for readout error. The mitigator object is the returned analysis result so the API docs should say something to that effect (eg characterizes local readout errors on qubit measurements. readout errors are defined as classical error on returned bitstrings blah blah... The returned analysis results is a ReadoutMitigator object which contains readout errors and can be used to apply readout error mitigation to measured counts etc).

For clarity I would also suggest just having two explicit experiments LocalReadoutError and CorrelatedReadoutError rather than a single one. Then the API docs can state exactly which characterization circuits are prepared in each case, and it is how they are used in practice anyway.

[chriseclectic]

This class is missing API documentation. It should have some description of what the experiment is and a reference to relevant paper and analysis class ie like this

[chriseclectic]

Why do these need to be stored as a class variable? You can just put the value in the method and allowed values should be listed in the API docs. If you make two separate experiment classes you also dont need it

[gadial]

I agree that making two separate experiments makes things much simpler; this is also addressed in #611

[chriseclectic]

Suggested change

	The currently supported mitigation methods are:
	Supported mitigation methods are:

[chriseclectic]

Though you can just remove this by making two experiments and putting all the relevent info into the experiments API docs

[chriseclectic]

This is not indented correctly

Suggested change

	and assumes readout errors are independent for each qubits
	and assumes readout errors are independent for each qubits

[chriseclectic]

Same here

[chriseclectic]

Ideally this should be more descriptive. On what the analysis is so someone not familiar with the experiment can read it and know what it does.

In this case it constructs a readout error assignment matrix for possibly correlated N-qubit measurment outcomes (and you can put in an equation). It should also mention what hte analysis results returned are (in this case the CorrelatedMitigator class and include a link to the terra API docs for that class).

[gadial]

Done

[chriseclectic]

Same here. This is missing proper API docs explaining what it does, what it returns, and link to the mitigator class returns.

[gadial]

Done

[chriseclectic]

Since these classes produce a figure that should also be mentioned in the API docs

[gadial]

Done

[chriseclectic]

The readout error experiments needsto be added to the doc string of this init file to show up in the API docs

[gadial]

Where do they fit in? Characterization experiments?

[chriseclectic]

I don't think this show up anywhere in the build docs because its not included in any other init files. The library section is where users see a list of experiments in the API docs.

[gadial]

Fixed

[gadial]

Thanks @chriseclectic , I've opened a new PR to address those issues:

#611