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

Cross Entropy Benchmarking sample and simulate two qubits #3706

Merged
merged 39 commits into from Feb 3, 2021

Conversation

mpharrigan
Copy link
Collaborator

Following #3669.

This wraps up the sampling and simulation functions teased in the previous notebook. It includes utilities for batching sampler requests and multiprocessing simulation. The notebook walks through how coherent error can cause fidelity to decay. In the next PR, we'll add an optimization loop to characterize the mystery angle.

@mpharrigan mpharrigan requested review from cduck, vtomole and a team as code owners January 26, 2021 03:20
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Jan 26, 2021
@mpharrigan
Copy link
Collaborator Author

anything else on this?

@mpharrigan mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 1, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 1, 2021
@CirqBot
Copy link
Collaborator

CirqBot commented Feb 1, 2021

Automerge cancelled: Merging is blocked (I don't understand why).

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 1, 2021
@@ -0,0 +1,283 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This notebook will eventually end up on the site. Please annotate the types with packages, e.g cirq.PhasedFSimGate - this will create automated links to the names on the reference docs.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cool, done

@mpharrigan
Copy link
Collaborator Author

@balopat back at you

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

I would still have to look at the code - though I trust @mrwojtek's review.
I have questions around adding tqdm.

@@ -14,3 +14,4 @@ sortedcontainers~=2.0
scipy
sympy
typing_extensions
tqdm
Copy link
Contributor

@balopat balopat Feb 1, 2021

Choose a reason for hiding this comment

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

New dependency alert!

In lieu of keeping cirq light, I'm hesitant to add tqdm as a dependency.

  1. We should check if this is going to break g3
  2. Can we make this somehow optional so that we reduce the dependency requirements for Cirq? (e.g. cirq[ui] or cirq[visualization] or via making the default value progress bar NoProgress)
  3. do we need to pin this?
  4. can we abstract it out better in our code instead of directly depending on it? That would help with testing progress logic as well as well as would help making it optional...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we're going to package our experiments/algorithms code in Cirq, we need to figure this out. For now, I can change it to default to no-progress but I think this hurts usability of this application code

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point about if we package this together we'll have to deal with this - and that is the plan.
I'll make a mental note of this. I checked 1.) and we should be fine (though I haven't run tests on that version).

@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 3, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 3, 2021
@CirqBot
Copy link
Collaborator

CirqBot commented Feb 3, 2021

Automerge cancelled: A required status check is not present.

Missing statuses: ['cla/google']

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 3, 2021
@mpharrigan
Copy link
Collaborator Author

Very strange. All the commits look like they're authored by googlers (?)

@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 3, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 3, 2021
@CirqBot CirqBot merged commit 1ed3176 into quantumlib:master Feb 3, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 3, 2021
@balopat
Copy link
Contributor

balopat commented Feb 3, 2021

I've seen this error before when cirqbot gets confused...we should have a look for cases when lack of "cla/google" is blamed for no reason and fix it in CirqBot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/xeb cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants