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

QV experiment #32

Merged
merged 61 commits into from
Jul 19, 2021
Merged

Conversation

dekelmeirom
Copy link
Contributor

@dekelmeirom dekelmeirom commented May 2, 2021

Summary

QV experiment class and QV analysis class

Details and comments

to do:

  • need to add tests

@dekelmeirom dekelmeirom self-assigned this May 2, 2021
@CLAassistant
Copy link

CLAassistant commented May 2, 2021

CLA assistant check
All committers have signed the CLA.

@dekelmeirom dekelmeirom changed the title [WIP] QV experiment QV experiment May 16, 2021
@ShellyGarion ShellyGarion requested a review from wshanks May 23, 2021 08:21
Copy link
Contributor

@yaelbh yaelbh left a comment

Choose a reason for hiding this comment

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

  1. Please add tests.
  2. In the notebook, the return QV is 0 (in the figure it says 8, but in the result it says 0).
  3. Find a way to remove the warnings from the notebook. I think this has to do with the experiment data holding experiments from different backends, which should be changed.

qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
@ShellyGarion ShellyGarion removed the request for review from wshanks May 27, 2021 14:51
@yaelbh
Copy link
Contributor

yaelbh commented Jun 2, 2021

Remember to put capital letters at the beginning of sentences, in comments and in the tutorial

@PetarJurcevic
Copy link

PetarJurcevic commented Jun 8, 2021

One thing I immediately noticed is that the QV-analysis flags success for trials <100. Per original QV definition in the paper, the number of trials has to be at least 100. This prob should be hardcoded.

But there seems to be something else going on. for example, even if we ignore the min_trials required, the method that spits out success/fail still flags success even though the conficence intervall is below 97.725% (z= 2). On top of that, one can calculate for a given avg HOP what is the min number of trials (again independent of min=100). I did that for the second example in the tutorial notebook:

  • confidence list: 0.9854888073253719
  • mean hop: 0.7851128472222222
  • sigma: 0.054253911286277816
  • depth: 3
  • trials: 45

however, when I rearrange the formula C3 from the original paper to get the min require trials I get 49 (instead of 45). Or the other way around, for 45 trials, the hop has to be 0.7884 to be within the z=2 one-sided confidence interval.

I believe none of the 3 examples in the notebook should have flagged success (even if we ignore the min=100 thing)

@coruscating coruscating added this to the Release 1 milestone Jun 14, 2021
@dekelmeirom
Copy link
Contributor Author

@PetarJurcevic thank you for noticing this error!
there was a mistake with the sigma calculation - instead of calculating:
(mean_hop * ((1.0 - mean_hop) / trials)) ** 0.5, we were calculating:
mean_hop * ((1.0 - mean_hop) / trials) ** 0.5 (the mean_hop was not inside the sqrt)
in the example in the tutorial, it made a difference of about 0.014 in the treshold - which made the test just pass.
the error was fixed and the example was updated

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.

Changing of run and _run_analysis signature here for two experiment data containers is problematic, so left some suggestions for how implement this using the base Experiment API.

There are also a few unnecessary methods for experiment options. To add trials I think you should just do:

# Run first batch of 50 trials
exp = QuantumVolume(trials=50)
data = exp.run(backend)
result1 = data.analysis_result(-1)

# Add another batch of 50 trials
exp.run(backend, experiment_data=data)
result2 = data.analysis_result(-1)

# Add last batch of 20 trials
exp.set_experiment_options(trials=20)
exp.run(backend, experiment_data=data)
result3 = data.analysis_result(-1)

qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_analysis.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_analysis.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
@ShellyGarion
Copy link
Contributor

@dekelmeirom - I'm trying to run the notebook from your PR and get a failure.
it's due to the following line in the code:
analysis_results, figures = self._run_analysis(experiment_data, **analysis_options)
it assumes that analysis_results should get a list of objects, but you provide it only a single object

@ShellyGarion
Copy link
Contributor

If the code is good, perhaps we can merge and open a seperate PR for the tests? (like in RB)

@yaelbh
Copy link
Contributor

yaelbh commented Jul 14, 2021

In my opinion every merged code must have at least some basic tests

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, though we should remove the use of the deprecated execute function and add some basic tests before merging.

class QuantumVolume(BaseExperiment):
"""Quantum Volume Experiment class

Experiment Options:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Experiment doc string needs expanding, but this can be follow up PR.

Copy link
Contributor

@ShellyGarion ShellyGarion Jul 15, 2021

Choose a reason for hiding this comment

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

I will handle it in a separate PR after this code is merged

qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
qiskit_experiments/quantum_volume/qv_experiment.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

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

LGTM. Great work Dekel!

Copy link
Contributor

@yaelbh yaelbh left a comment

Choose a reason for hiding this comment

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

Comments about the tutorial:

  1. "which will effect" -> affect.
  2. There is a warning in the tutorial.
  3. "to be consider" -> consider.
  4. Add periods at the the end of sentences.
  5. "less than more" -> less than 100 additional.
  6. "using batch" -> using a batch
  7. "with increasing" -> an increasing
  8. "for specific" -> the specific, then the colon should be at the same line.
  9. improvments -> improvements (not sure if this is the correct word here, maybe "enhancements"). Run a spell checker.
  10. applyed -> applied.
  11. Start sentences with capital letters.

@chriseclectic chriseclectic merged commit e7c9ba5 into qiskit-community:main Jul 19, 2021
paco-ri pushed a commit to paco-ri/qiskit-experiments that referenced this pull request Jul 11, 2022
Co-authored-by: Gadi Aleksandrowicz <gadia@il.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.