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

Removing noise model check in Simulator initialization #4216

Merged
merged 10 commits into from
Jun 21, 2021

Conversation

asmuzsoy
Copy link
Contributor

Addresses issue #4170 by removing NOISE_MODEL_LIKE check in sparse_simulator.py and updates tests accordingly.

@asmuzsoy asmuzsoy requested review from cduck, vtomole and a team as code owners June 16, 2021 13:29
@asmuzsoy asmuzsoy requested a review from maffoo June 16, 2021 13:29
@asmuzsoy asmuzsoy changed the title Noise models 4170 Removing noise model check in Simulator initialization Jun 16, 2021
@google-cla
Copy link

google-cla bot commented Jun 16, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jun 16, 2021
@95-martin-orion 95-martin-orion requested review from viathor and removed request for maffoo June 16, 2021 17:13
@google-cla
Copy link

google-cla bot commented Jun 16, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@asmuzsoy
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes Makes googlebot stop complaining. and removed cla: no labels Jun 17, 2021
Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

Please merge the master branch back into this one.

result1 = simulator.run(circuit, repetitions=50)
result2 = simulator.run(circuit, repetitions=50)

assert result1 != result2
Copy link
Collaborator

@viathor viathor Jun 17, 2021

Choose a reason for hiding this comment

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

I suppose for this PR we just want to verify that the simulator accepts the noise model, as opposed to raising an exception. However, we can make the test more useful by checking that the measurement falls into an interval (which ideally would be verified by a calculation independent of the code under test).

cirq-core/cirq/sim/sparse_simulator_test.py Show resolved Hide resolved
cirq.Simulator(noise=cirq.amplitude_damp(0.5))
def test_noise_model():
q = cirq.LineQubit(0)
circuit = cirq.Circuit(cirq.I(q), cirq.measure(q))
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a class of bugs that make sum(result.measurements['0'])[0] equal to zero exactly (e.g. one such bug is the simulator accepting but ignoring the noise). These bugs would pass undetected by the test.

Suggestion: Replace identity with cirq.H(q) and change the assert to something like

assert 40 <= sum(result.measurements['0'])[0] < 60

Feel free to use tighter bounds, I just made those up. They should be gapped away from zero on the left and away from 100 on the right, though.

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 for the suggestion, I just updated it!

@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 21, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 21, 2021
@CirqBot CirqBot merged commit ba2cd7f into quantumlib:master Jun 21, 2021
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 21, 2021
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 21, 2021
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Addresses issue quantumlib#4170 by removing NOISE_MODEL_LIKE check in sparse_simulator.py and updates tests accordingly.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants