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 a projector observable to QiskitDevice #137

Merged
merged 7 commits into from
May 28, 2021

Conversation

wongwsvincent
Copy link
Contributor

Context:
Adding a projector observable to QiskitDevice.

Description of the Change:
Adding Projector to the list of observables compatible with QiskitDevice in qiskit_device.py.

Related GitHub PR:
Work coherently with PR #1356

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 @wongwsvincent 🙂

This looks great, and likely doesn't need a unit test (unlike the Cirq device) since there is no new logic here.

I do have one question though, how come you added docstrings to some of the inherited methods? Not having docstrings here was intentional, since Sphinx will instead insert the docstrings from the parent class. This reduces the maintainability overhead :)

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -102,7 +102,7 @@ class QiskitDevice(QubitDevice, abc.ABC):
that support returning the underlying quantum statevector"""

operations = set(_operation_map.keys())
observables = {"PauliX", "PauliY", "PauliZ", "Identity", "Hadamard", "Hermitian"}
observables = {"PauliX", "PauliY", "PauliZ", "Identity", "Hadamard", "Hermitian", "Projector"}
Copy link
Member

Choose a reason for hiding this comment

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

💪

@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #137 (97e0b11) into master (5793462) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #137   +/-   ##
=======================================
  Coverage   99.29%   99.29%           
=======================================
  Files           7        7           
  Lines         284      284           
=======================================
  Hits          282      282           
  Misses          2        2           
Impacted Files Coverage Δ
pennylane_qiskit/qiskit_device.py 100.00% <100.00%> (ø)

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 5793462...97e0b11. Read the comment docs.

Co-authored-by: Josh Izaac <[email protected]>
@wongwsvincent
Copy link
Contributor Author

@josh146 , I added docstrings here because I saw CodeFactor in the github workflow was complaining about not having docstrings under the functions. I'm not sure why the docstrings weren't inherited from the parent class, and I'm not familiar enough with Sphinx to understand why. The flagged issues can be found here.

Could you give me a bit of help here?

@josh146
Copy link
Member

josh146 commented May 28, 2021

I saw CodeFactor in the github workflow was complaining about not having docstrings under the functions

Thanks for flagging this @wongwsvincent --- it appears that Codefactor is incorrect here, and these particular methods do not need docstrings.

Feel free to remove them, even if codefactor complains about the missing docstrings, I will be able to overwrite those :)

@wongwsvincent
Copy link
Contributor Author

Thanks for clarifying, @josh146 ! I have removed the docstrings.

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 @wongwsvincent! Good to go now 💯

@josh146 josh146 merged commit b135c4e into PennyLaneAI:master May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants