qiskit icon indicating copy to clipboard operation
qiskit copied to clipboard

Leverage rust marginalization function for marginal_counts()

Open mtreinish opened this issue 3 years ago • 5 comments

Summary

In #8026 we added a new marginalization function marginal_distribution() which was a standalone marginalization function for counts and distribution dictionaries. The core of that new function was written in rust for performance. In that PR we didn't use the rust core because it's behavior was slightly different especially around the order of indices. This commit reworks the internals of the marginal_counts() function to leverage the same rust core function to ensure the ordering is consistent after the migration the index sort is changed.

Details and comments

mtreinish avatar May 31 '22 18:05 mtreinish

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

qiskit-bot avatar May 31 '22 18:05 qiskit-bot

I've marked this as on hold because I'd like to wait until #8051 merges so we have a rust bin->hex converter so we can add an option to the rust marginal_counts() function to natively return hex keys and avoid the second iteration to convert to hex strings

mtreinish avatar May 31 '22 18:05 mtreinish

Pull Request Test Coverage Report for Build 2542021781

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 84.31%

Totals Coverage Status
Change from base Build 2539968332: 0.003%
Covered Lines: 54970
Relevant Lines: 65200

💛 - Coveralls

coveralls avatar May 31 '22 19:05 coveralls

I've removed the on hold from this, I was misremembering #8051 and it doesn't have an efficient rust bin to hex converter (it has a hex to bin converter). Let me quickly look at adding a hex return option to the rust marginal distribution functions quickly right now but we shouldn't necessarily block on it since I doubt the extra iteration will be a real bottleneck.

mtreinish avatar Jun 22 '22 11:06 mtreinish

The commit says that the order of results stays the same (I think), but I ran a brief test, and seemed to get different results if indices is not None. I could be using the functions wrong, and I'm not 100% certain what guarantees you were talking about, though. For reference, I did this:

import itertools
from qiskit.result import marginal_counts

counts = [(f"{i:03b}", 1 << i) for i in range(1 << 3)]
indices = [None, [0], [0, 1], [1, 0]]
marginals = [
    marginal_counts(dict(cs), idx)
    for cs, idx in itertools.product(itertools.permutations(counts), indices)
]

The input and output orders don't agree in most cases for the non-None indices.

The order I was talking about was basically that indices=[0, 1] is treated identically to indices=[1, 0]. The context around that was #6230 which was complaining that marginal_counts() didn't respect the order of the indices in the output bitstrings (the issue wanted to use indices=[1, 0] to permute the bits). I'm not actually sure it preserves the insertion order in the output generated.

Honestly we can hold off on this PR at this point, it'd be nice to have since it'll speed things up for marginal_counts() but that's far from critical and there are definitely backwards compat questions still because I wanted this to be indistinguishable for the most part for existing users.

mtreinish avatar Jun 22 '22 13:06 mtreinish

This is too stale, I'm just going to close this. It was never critical I'll open up a different PR to update the documentation around the function to just point people towards marginal distribution instead of marginal counts.

mtreinish avatar Mar 31 '23 16:03 mtreinish