Use marginal_distribution() instead of marginal_counts()

[mtreinish]

Summary

This commit updates the Qiskit marginalization function used in qiskit-experiments. Qiskit has 2 different function available for marginalizing counts: marginal_counts() [1] which is designed to work with Counts or Result objects and does not respect ordering of the indices passed in. The other is marginal_distribution() [2] which is designed to work with Counts, Probabilities, QuasiProbabilities, or a dictionary with string keys and integer or float values that respects the order of the indices passed in (so it can be used to permute the bits). In general if you're not working with a Result object it is better to use marginal_distribution() because it is written primarily in Rust and 10-100x faster than marginal_counts().

Details and comments

[1] https://qiskit.org/documentation/stubs/qiskit.result.marginal_counts.html
[2] https://qiskit.org/documentation/stubs/qiskit.result.marginal_distribution.html

[mtreinish]

The other function which I thought we added for experiments but I didn't see anything that looked similar through some quick grepping is marginal_memory: https://qiskit.org/documentation/stubs/qiskit.result.marginal_memory.html#qiskit.result.marginal_memory which is used for marginalizing different results levels. If there is a need for that kind of marginalization using that qiskit function would be a good choice because it's also written in Rust and also multithreaded.

[mtreinish]

Oh interesting it's failing because the value type is np.int64 instead of a python int. That hasn't come up before, it's an easy fix on the qiskit-side to add support to this (especially because there isn't any type conversion needed in that case as int64->i64 directly). But in the meantime I'll just cast it to an int.

[nkanazawa1989]

Thanks Matthew. This looks good to me. In my local profiling with a composite curve analysis for parallel 27 qubit Rabi experiment, this PR reduces total processing time from 15.0s to 9.26s (roughly 40% time reduction). This is awesome improvement.

Since Qiskit/qiskit#9976 is going to be merged, do you want to remove safety typecast and turn this into on hold until next Terra release? Otherwise I'll merge.

[mtreinish]

I guess it depends on whether you think we should backport this for a potential 0.5.1 or not? If you want to include it for 0.5.1 and backport it we should leave it as is. But if you just want to save it for 0.6.0 lets just wait for the terra release when we can bump the minimum qiskit-terra version to 0.24.0 and remove the type cast.

[nkanazawa1989]

I think this should be released with 0.6 because this marginal_counts code has been existing from 0.1 (not a bugfix for 0.5). But this PR is a good motivation for us to cut next release soon :)

[mtreinish]

Ok, then I'll mark this as on hold then. I'll update this after the 0.24.0 release in a a couple of weeks to remove the type cast and update the minimum terra version to 0.24.0

[coruscating]

We decided to include this in the 0.5.2 release since users have been asking for more performance improvements on large experiments. Once Terra 0.24 is released and this is updated and merged I'll tag the patch release. Can you also add a release note when you update this @mtreinish?

[mtreinish]

I think for a patch release you probably don't want to bump the minimum qiskit-terra version for this (unless you need to for other reasons) just so you're not requiring people to bump their installed terra version to get the fixes in 0.5.2.

[coruscating]

@mtreinish You're right, I got mixed up there, our decision was actually to include this in the 0.5.2 release without bumping the required terra version to 0.24. @nkanazawa1989 are you ok with merging this as is?

[nkanazawa1989]

Please merge this for patch release. Once we bump terra version we should remove the typecast logic (it is another slight performance regression).