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

QiskitDevice.probabilities() changed to QiskitDevice.probability() to… #80

Merged
merged 4 commits into from
Apr 15, 2020

Conversation

rafaelha
Copy link
Contributor

… restore compatibility with pennylane python package

… restore compatibility with pennylane python package
@antalszava
Copy link
Contributor

Hi @rafaelha, thank you so much for your pull request! 🎊

Adjusting the name of the QiskitDevice.probabilities method is a change that is upcoming in this plugin as we are porting to the use of QubitDevice from PennyLane.

I'd be curious, was there perhaps a bug or any problems that you have faced while using QiskitDevice.probabilities?

@rafaelha
Copy link
Contributor Author

Hi @rafaelha, thank you so much for your pull request! 🎊

Adjusting the name of the QiskitDevice.probabilities method is a change that is upcoming in this plugin as we are porting to the use of QubitDevice from PennyLane.

I'd be curious, was there perhaps a bug or any problems that you have faced while using QiskitDevice.probabilities?

Hi, yes, running a qnode on a qiskit device that returns qml.probs raises NotImplementedError: Returning probability not currently supported by qiskit.ibmq. This seems to be because pennylane calls the function Device.probability (https://github.com/XanaduAI/pennylane/blob/a776dbecfb0c1d32bb04012a69ee0e559d58c3b5/pennylane/_device.py) instead of QiskitDevice.probabilities. Renaming QiskitDevice.probabilities to QiskitDevice.probability fixed that error for me. Cheers!

@antalszava
Copy link
Contributor

I see that makes sense! Thank you so much once again for your addition! 😊

The tests are failing due to the test suite pulling the newest version (0.18) of Qiskit. We are investigating this right now and creating a fitting solution.

@antalszava antalszava mentioned this pull request Apr 15, 2020
Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Thanks so much again for this change @rafaelha! 💯 😊

Opened two related pull requests:

Checked locally, the test from #82 picks up the case that was mentioned and the changes from this PR made it pass for me locally!

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.

Thanks @rafaelha! @antalszava, it looks like we are ready to merge in #80, #81, and #82. Do we want to update the changelog separately, since these three PRs are all somewhat related?

@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #80 into master will decrease coverage by 0.58%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
- Coverage   99.12%   98.53%   -0.59%     
==========================================
  Files           7        7              
  Lines         341      341              
==========================================
- Hits          338      336       -2     
- Misses          3        5       +2     
Impacted Files Coverage Δ
pennylane_qiskit/qiskit_device.py 98.63% <100.00%> (ø)
pennylane_qiskit/ibmq.py 91.30% <0.00%> (-8.70%) ⬇️

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 dfba326...2887b4c. Read the comment docs.

@antalszava
Copy link
Contributor

Thanks for catching that @josh146!

@rafaelha, could you please modify the CHANGELOG.md file such that it includes:

  • a one-sentence description of this PR (for example in the Bugfix section);
  • yourself as a contributor in the list of contributors for the current release (# Release 0.9.0-dev).

Thanks! 😊

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.

Thanks @rafaelha, updated changelog looks perfect!

@josh146 josh146 merged commit 9b462af into PennyLaneAI:master Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants