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

No noise model reset #114

Merged
merged 10 commits into from
Nov 11, 2020
Merged

No noise model reset #114

merged 10 commits into from
Nov 11, 2020

Conversation

antalszava
Copy link
Contributor

@antalszava antalszava commented Nov 6, 2020

Context
Upon device initialization, a noise model can be passed to a Qiskit Aer device by specifying the noise_model keyword argument. The noise model is then stored as an attribute, which is initialized to None and is further reset to None once dev.reset is called.

PennyLane core calls dev.reset in certain places (e.g., as part of QNode evaluation) which eventually result in "losing" the user defined noise model.

Furthermore, Qiskit Aer devices persist the options that were set for them prior. This affects consecutive device creations: e.g., devices created after a noisy device creation would also have noise be set as an option.

Changes

  • This PR removes the noide_model attribute and moves the dev.backend.set_options call directly to QiskitDevice.__init__ from QiskitDevice.run.
  • Adds a call to the clear_options method for Aer backends

@antalszava antalszava changed the title Don't reset the noise model No noise model reset Nov 6, 2020
@antalszava antalszava added the bug Something isn't working label Nov 6, 2020
@antalszava antalszava marked this pull request as ready for review November 6, 2020 22:07
Copy link
Member

@josh146 josh146 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 @antalszava, however it seems that this is causing a large number of tests to fail 🤔

Are some of the tests implicitly written assuming noise models are being reset?

qml.RX(0, wires=[0])
return qml.expval(qml.PauliZ(wires=0))

assert circuit() == -1
Copy link
Member

Choose a reason for hiding this comment

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

nice test :)

@glassnotes
Copy link
Contributor

I get two failing tests (running 0.13.0 dev for both PennyLane and PennyLane-Qiskit):

=========================================================================================================================== short test summary info ===========================================================================================================================
FAILED test_ibmq.py::test_account_error - Failed: DID NOT RAISE <class 'qiskit.providers.ibmq.exceptions.IBMQAccountError'>
FAILED test_sample.py::TestTensorSample::test_hermitian[AerDevice-qasm_simulator-8192-False-1.0-1.0-1.0] - AssertionError: assert False

But neither of these seem to involve the noise model (at least not directly).

In terms of the actual fix - my code that was previously not working due to the noise model being reset is now working as expected; the noise model persists between jobs.

@codecov
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

Merging #114 (4d7899a) into master (7ba24ac) will increase coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   97.92%   98.26%   +0.34%     
==========================================
  Files           7        7              
  Lines         289      289              
==========================================
+ Hits          283      284       +1     
+ Misses          6        5       -1     
Impacted Files Coverage Δ
pennylane_qiskit/qiskit_device.py 98.13% <100.00%> (+0.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ba24ac...4d7899a. Read the comment docs.

@antalszava antalszava merged commit 14b7d91 into master Nov 11, 2020
@antalszava antalszava deleted the no_reset_noise_model branch November 11, 2020 22:25
@josh146
Copy link
Member

josh146 commented Nov 13, 2020

@antalszava not sure if I missed it, but don't forget to update the changelog!

@antalszava
Copy link
Contributor Author

@josh146 thanks for the reminder :) Added it in #116.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants