Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leverage rust marginalization function for marginal_counts() #8122

Closed
wants to merge 4 commits into from

Conversation

mtreinish
Copy link
Member

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

In Qiskit#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.
@mtreinish mtreinish added on hold Can not fix yet Changelog: None Do not include in changelog labels May 31, 2022
@mtreinish mtreinish added this to the 0.21 milestone May 31, 2022
@mtreinish mtreinish requested a review from a team as a code owner May 31, 2022 18:52
@qiskit-bot
Copy link
Collaborator

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

@mtreinish
Copy link
Member Author

mtreinish commented May 31, 2022

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

@coveralls
Copy link

coveralls commented May 31, 2022

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

@jakelishman jakelishman self-assigned this Jun 21, 2022
@mtreinish mtreinish removed the on hold Can not fix yet label Jun 22, 2022
@mtreinish
Copy link
Member Author

mtreinish commented Jun 22, 2022

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.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 188 to +190
# Sort the indices to keep in descending order
# Since bitstrings have qubit-0 as least significant bit
indices = sorted(indices, reverse=True)

# Build the return list
new_counts = Counter()
for key, val in counts.items():
new_key = "".join([_remove_space_underscore(key)[-idx - 1] for idx in indices])
new_counts[new_key] += val
return dict(new_counts)
return results_rs.marginal_counts(counts, sorted(indices))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment probably wants removing as well, I guess?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the comment in place because it just applies to the inline sorted(indices) instead of doing indices = sorted(indices, reverse=True). They're doing the same functional thing to preserve the independence of index of interest ordering from the output (basically don't permute the bits if you did something like [1, 0]).

@mtreinish
Copy link
Member Author

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 mtreinish modified the milestones: 0.21, 0.22 Jun 22, 2022
@mtreinish mtreinish removed this from the 0.22 milestone Sep 13, 2022
@mtreinish
Copy link
Member Author

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 mtreinish closed this Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants