qiskit
qiskit copied to clipboard
Leverage rust marginalization function for marginal_counts()
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
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
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
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 | |
|---|---|
| Change from base Build 2539968332: | 0.003% |
| Covered Lines: | 54970 |
| Relevant Lines: | 65200 |
💛 - 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.
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
indicesis notNone. 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-
Noneindices.
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.
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.