-
Notifications
You must be signed in to change notification settings - Fork 127
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
QV experiment #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please add tests.
- In the notebook, the return QV is 0 (in the figure it says 8, but in the result it says 0).
- 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.
Remember to put capital letters at the beginning of sentences, in comments and in the tutorial |
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:
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) |
@PetarJurcevic thank you for noticing this error! |
There was a problem hiding this 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)
@dekelmeirom - I'm trying to run the notebook from your PR and get a failure. |
Qv experiment
If the code is good, perhaps we can merge and open a seperate PR for the tests? (like in RB) |
In my opinion every merged code must have at least some basic tests |
There was a problem hiding this 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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!
There was a problem hiding this 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:
- "which will effect" -> affect.
- There is a warning in the tutorial.
- "to be consider" -> consider.
- Add periods at the the end of sentences.
- "less than more" -> less than 100 additional.
- "using batch" -> using a batch
- "with increasing" -> an increasing
- "for specific" -> the specific, then the colon should be at the same line.
- improvments -> improvements (not sure if this is the correct word here, maybe "enhancements"). Run a spell checker.
- applyed -> applied.
- Start sentences with capital letters.
Co-authored-by: Gadi Aleksandrowicz <[email protected]>
Summary
QV experiment class and QV analysis class
Details and comments
to do: