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

Level2 memory #801

Merged
merged 16 commits into from
May 5, 2022
Merged

Level2 memory #801

merged 16 commits into from
May 5, 2022

Conversation

eggerdj
Copy link
Contributor

@eggerdj eggerdj commented May 4, 2022

Summary

This PR fixes #800

Details and comments

The fix is done by adding proper memory marginalization for level 2. We distinguish between level 1 and level 2 using list vs string. This code can be tested on hardware by running the following:

from qiskit_experiments.library import FineXAmplitude
from qiskit_experiments.framework import ParallelExperiment
from qiskit import IBMQ

backend = ...

experiments = []
for qubit in [0, 1]:
    fine_amp = FineXAmplitude(qubit, backend)
    fine_amp.enable_restless()
    experiments.append(fine_amp)
    
parallel_exp = ParallelExperiment(experiments, backend)
parallel_exp.set_run_options(**experiments[0].run_options.__dict__)

exp_data = parallel_exp.run()

With the changes in this PR we get the following output:

exp_data.component_experiment_data(0).figure(0)

image
and

exp_data.component_experiment_data(1).figure(0)

image

Copy link
Collaborator

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

Some suggestions for cleaning up code a little bit

)
header = {"memory_slots": num_cbits}
marginalized_memory = []
for shot in datum["memory"]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since ParallelExperiments always concatenate clbits for sub-experiments you are garuenteed that the composite_clbits are a continuous range. In this case you can simplify this processing like:

clbits = composite_bits[i]
idx = slice(clbits[0], clbits[-1] + 1)
marginalized = [format_counts_memory(mem, header)[idx] for mem in datum["memory"]]

If performance becomes an issue for large numbers of shots (which im sure it will at some point) we should probably update or replace the format_counts_memory into a more efficient vectorized function that can go straight from list/array hex str -> list/array marginal counts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. See here: 871400e note that you still need the [::-1] to conform to Qiskit's bit ordering. Unless the composite_bits already account for this which I doubt? Note that before 871400e I realized that we were calling format_counts_memory a total of n_shots x n_subexperiments times. This is inefficient and by pulling this out of the loop over for i, index in enumerate(metadata["composite_index"]) we only need to call it n_shots times which is much better.

"".join(shot[idx] for idx in composite_clbits[i])[::-1]
)
sub_data["memory"] = marginalized_memory
# level 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add a test for level-1 marginalization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here: 5b3eee0 This also required adding a fix to be able to handle single-shot memory vs averaged memory.

test/test_composite.py Outdated Show resolved Hide resolved
# level 2
if f_memory is not None:
idx = slice(composite_clbits[i][0], composite_clbits[i][-1] + 1)
sub_data["memory"] = [shot[::-1][idx][::-1] for shot in f_memory]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot about the reversed bit indexing in my slice example. I think you can remove the need to reverse and re-reverse with ::-1 if you build the slice with that already in mind. I tried some quick tests and I think this is correct:

idx = slice(-1 - composite_qubits[i][-1], -composite_qubits[i][0] or None)
sub_data["memory"] = [shot[idx] for shot in f_memory]

The None case is needed to handle when a qubit[0] = 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's better thanks. See here: f9eefda

@chriseclectic chriseclectic added backport stable potential The issue or PR might be minimal and/or import enough to backport to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels May 5, 2022
Copy link
Collaborator

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

Looks good, its just missing a release note now saying what the "bug fix" was.

@eggerdj eggerdj requested a review from chriseclectic May 5, 2022 21:25
@chriseclectic chriseclectic merged commit fa2502f into qiskit-community:main May 5, 2022
@eggerdj eggerdj deleted the level2_memory branch May 6, 2022 07:13
chriseclectic pushed a commit to chriseclectic/qiskit-experiments that referenced this pull request May 18, 2022
paco-ri pushed a commit to paco-ri/qiskit-experiments that referenced this pull request Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport stable potential The issue or PR might be minimal and/or import enough to backport to stable Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ParallelExperiment does not work in a restless setting
2 participants