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

Add ZerosSampler #2912

Merged
merged 36 commits into from
Jul 16, 2020
Merged

Add ZerosSampler #2912

merged 36 commits into from
Jul 16, 2020

Conversation

fedimser
Copy link
Collaborator

A dummy sampler which implements cirq.Sampler and returns zeroes for all measurements.

Fixes #2818

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Apr 15, 2020
Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

This looks good.

  1. Could you add the async sampler methods as well? not blocking but I personally use these methods in my workflows
  2. Right now this can be used for a simple check that there are no glaring errors in one's experimental workflow. This could be hugely more useful if it could also validate that you're using supported gates and extant qubits. Unfortunately, right now the gateset validation logic is trapped inside the Quantum Engine serialization routines, but I encourage you to think about how to incorporate gateset validation into ZerosSampler


def test_sample():
# Create a circuit whose measurements are always zeros, and check that
# results of ZeroSampler on this circuit are identical to results of
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

cirq/work/zeros_sampler.py Outdated Show resolved Hide resolved
@fedimser
Copy link
Collaborator Author

fedimser commented Apr 18, 2020

Hi Matthew,

  1. ZeroSampler inherits from Sampler, which has default implementation of run_async which calls run and of run_sweep_async which calls run_sweep. So now async methods of ZerosSampler will work as expected, and I don't need to implement them. Am I right?
  2. I didn't work with gate sets in Cirq before, but here is what I can suggest. We can pass SerializableGateSet object to ZeroSampler constructor. Then in run_sweep we will check for all operations whether it is_supported_operation. And if there is no gate set, don't do this validation. Does it make sense? Do you think it's better to do this in a follow-up PR?
    Also, what are "extant qubits"?

@fedimser fedimser added the Ready for Re-Review For when reviewers take their time. label Apr 22, 2020
@mpharrigan
Copy link
Collaborator

1: ok good
2: that sounds entirely reasonable to me
2b, re: follow up PR. up to you. If it's not too drastic of a change, I'd say just put it into this because it will likely involve changing the API of ZerosSampler slightly
2c: re: extant qubits. real devices only have certain qubits and a common failure mode is accidentally using e.g. a LineQubit when you should use GridQubits or using GridQubit(0,0) when the physical numbering scheme starts somewhere else

@fedimser
Copy link
Collaborator Author

fedimser commented May 6, 2020

Hi @mpharrigan,
I added gateset validation, PTAL.

Regarding extand qubits: there is a class cirq.devices.Device, which has method qubit_set. So we could pass the device to a constructor (like I did with gate set) and in run_sweep() get all qubits referenced by a device, and check that they are all in this qubit_set.
However, this can be simplified: we could just pass qubit_set. Or, if we are passing device, we can just call Device.validate_circuit.
What do you prefer?
Also, does Device.validate_circuit validates gateset? If so we don't need SerializableGetSet in ZerosSampler (which I just added).

@dabacon
Copy link
Collaborator

dabacon commented May 16, 2020

Question to @mpharrigan : shouldn't the device and gateset validation be handled by setting an appropriate device on the circuit? What is the primary use case for this class, is it to do this validation? Then maybe it should be called something slightly different?

Copy link
Collaborator

@dabacon dabacon left a comment

Choose a reason for hiding this comment

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

A couple of drive by comments.

cirq/work/zeros_sampler.py Outdated Show resolved Hide resolved
cirq/work/zeros_sampler_test.py Show resolved Hide resolved
@fedimser
Copy link
Collaborator Author

fedimser commented Jun 9, 2020

Can we merge this?
@mpharrigan @dabacon

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.

LGTM

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

CirqBot commented Jul 11, 2020

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 Jul 11, 2020
@dabacon dabacon added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 16, 2020
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 16, 2020
@CirqBot
Copy link
Collaborator

CirqBot commented Jul 16, 2020

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 Jul 16, 2020
@balopat balopat removed the cla: yes Makes googlebot stop complaining. label Jul 16, 2020
@balopat
Copy link
Contributor

balopat commented Jul 16, 2020

@googlebot please rescan

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Jul 16, 2020
@dabacon dabacon merged commit 8afb02c into quantumlib:master Jul 16, 2020
tonybruguier pushed a commit to tonybruguier/Cirq that referenced this pull request Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining. Ready for Re-Review For when reviewers take their time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZerosSampler
7 participants