Migrate measure mitigation from ignis for QuantumInstance

[mtreinish]

Summary

This commit migrates the measurement mitigation code from qiskit-ignis
into qiskit-terra for use with the QuantumInstance class. The
QuantumInstance class's usage of the measurement mitigation from ignis
is the one thing blocking us from deprecating qiskit-ignis completely.
By embedding the code the quantum instance depends on inside
qiskit.utils users of QuantumInstance (and therefore qiskit.algorithms)
can construct and use measurement mitigation in it's current form. The
use of this migrated module is only supported for use with the
QuantumInstance class and is explicitly documented as internal/private
except for how it gets used by the QuantumInstance.

There is ongoing work to create a standardized mitigation API in #6485
and #6748, this does not preclude that work, but we should adapt this as
part of those efforts to use the standardized interface. Ideally this
would have been made a private interface and not exposed it as user facing
(in deference to the standardization effort), but unfortunately the
QuantumInstance expects classes of these classes as it's public interface
for selecting a mitigation technique which means users need to be able to
use the classes. However, as only the classes are public interfaces we
can adapt this as we come up with a standardized mitigation interface
and rewrite the internals of this and how the QuantumInstance leverages
mitigators to use the new interface.

Details and comments

A good follow-up here would be to adapt the mitigator selection kwarg to
deprecate the use of classes and then we can make things explicitly
private in the migrated code and wait for #6495 and #6748 to be ready
for our user facing API. I opted to not include that in this PR to
minimize changes to just what we migrated from ignis and update usage of
old ignis classes to rely on the migrated version.

[omarshehab]

@ShellyGarion , thanks for mentioning this PR in #6980. Am I correct to understand that this PR is yet to be merged and not specifically dated for client release yet?

[jakelishman]

I attempted not to comment much on what the code is actually doing, since it's just copied over from Ignis (though I did leave ~two in that vein).

I reviewed by diffing against the Ignis version, so I've commented wherever there's a difference (barring obvious pylint/black changes and functions being pulled from one file to another). A couple of those comments just say "I think this is ok to change, this is just to make sure we changed it deliberately", but a couple have extra stuff about potential issues with the deprecation procedure.

The tests seems to be amalgamated from a couple of places and chopped and changed a bit more, so my diffing didn't work as cleanly. Other than the comment about test inheritance that seems a bit off, the tests appear to be copied faithfully.

The rest of the comments are just typo fixes or comments on the documentation, and I think this will be ok to bring in shortly.

[jakelishman]

Do we need the same attempt to import from Ignis for this function (build_measurement_error_mitigation_qobj) as we did in the previous one (build_measurement_error_mitigation_circuits)?

[mtreinish]

it's not strictly necessary because the calibration circuits are the same. Above where we handle the ignis path we're actually just passing the terra version of the classes here.

If ignis were going to continue active feature development I would have added handling for calling ignis here just in case the cal circuits diverged, but since everything is pretty much froze and this is just for compat while the api for mitigation in algorithms is being redone I think this is fine.

[jakelishman]

My understanding is that the MeasurementFilter and TensoredFilter classes are meant to be entirely private, and users (and other parts of Qiskit except the two fitters here) shouldn't be creating or instantiating them at all. For example, there's no generated autosummary pages for them. Is it worth renaming their module to _filters.py, and not importing them in this __init__.py to follow Python conventions of public/private access and make the access pattern clear to consumers of Terra who aren't reading the documentation?

[mtreinish]

Yeah, that's not a bad idea. I was trying to avoid the _module.py pattern mostly because in the dark before times (i.e. before I first started working on Qiskit) pretty much every module was _foo.py and the user api was defined as the re-exported classes from the __init__.py (without __all__). See https://github.com/Qiskit/qiskit-terra/tree/0.5.0/qiskit if you'd like to get a more concrete feel for what I'm describing. It's left a bad taste ever since so I just try to avoid it so people aren't tempted to revert to that "model". But here I think it makes sense I'll update this accordingly.

[jakelishman]

Haha, that's quite the structure. It feels like there's valid intent behind it (perhaps attempting to force a canonical import path?), but yeah, that's gone pretty overboard.

[jakelishman]

In the Ignis version of this test, these results are loaded from a saved file. I timed it on my machine, and this whole test takes under a second to run, so I think we're fine to generate the results like this.

[mtreinish]

Yeah it's an annoying pattern in ignis tests where they use json files committed to the repo for testing the fitters where the generation of the data is quite quick. I tried to remove all of that whenever I encountered it in the tests since it was unnecessary complexity (and git repo bloat).

[jakelishman]

Again, in Ignis this test loads from a known file, but generating the results like this takes well under a second, so I think we're fine.

[jakelishman]

I'm happy this is a correct import of the code from Ignis, and we're handling exposing it on the Terra side fine. The code itself likely wouldn't pass Terra review, but that's beyond the scope of this PR, since we just need this in place to completely deprecate Ignis.