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

Update the developers plugin guide to use QubitDevice #483

Merged
merged 46 commits into from
Jan 29, 2020

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Jan 24, 2020

Context: The new QubitDevice makes plugin development significantly easier. The existing Device API should be considered deprecated, and will eventually be removed from Device.

Description of the Change:

  • Updates the plugin guide to focus on the new plugin API

  • Cleans up some docstrings in QubitDevice

Benefits: n/a

Possible Drawbacks:

  • Some plugins currently still use Device; they will need to be ported to QubitDevice.

  • A CVDevice equivalent is still TODO.

Related GitHub Issues: n/a

@josh146 josh146 added devices 💻 Device or plugin API related documentation 📘 Documentation changes and updates WIP 🚧 Work-in-progress labels Jan 24, 2020
@codecov
Copy link

codecov bot commented Jan 24, 2020

Codecov Report

Merging #483 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #483   +/-   ##
======================================
  Coverage    99.1%   99.1%           
======================================
  Files          51      51           
  Lines        3910    3910           
======================================
  Hits         3875    3875           
  Misses         35      35

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 4b5543c...7b43cca. Read the comment docs.

@josh146 josh146 removed the WIP 🚧 Work-in-progress label Jan 26, 2020
Copy link
Contributor

@mariaschuld mariaschuld left a comment

Choose a reason for hiding this comment

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

Awesome Josh, I haven't had much to do with plugin development but now I feel I could do it myself!

Two general things you may want to improve:

  1. The "Device execution" section is still rather dense. Maybe use a few more sentences/examples to explain, since people also use different technical terms sometimes?
  2. It is a bit confusing that there are CV/Qubit devices, then the QubitDevice is used throughout the explanation, and at the end there is a CVOperation. I think it's fine to focus on QubitDevice, but it should be clear how CV is different from that?

doc/development/plugins.rst Show resolved Hide resolved
doc/development/plugins.rst Outdated Show resolved Hide resolved
doc/development/plugins.rst Outdated Show resolved Hide resolved
doc/development/plugins.rst Show resolved Hide resolved
doc/development/plugins.rst Outdated Show resolved Hide resolved
doc/development/plugins.rst Outdated Show resolved Hide resolved
doc/development/plugins.rst Outdated Show resolved Hide resolved
@josh146
Copy link
Member Author

josh146 commented Jan 27, 2020

It is a bit confusing that there are CV/Qubit devices, then the QubitDevice is used throughout the explanation, and at the end there is a CVOperation. I think it's fine to focus on QubitDevice, but it should be clear how CV is different from that?

This was (unfortunately) deliberate 🙁 My thinking was:

  • Almost all external plugin developers will be working with the qubit model

  • There is no CVDevice yet; it is planned, but won't make it to this release.

What do you think is the least confusing approach? Once the CVDevice is ready, my plan was to create a CV device page. But it does raise questions in the interim.

Options are:

  • Add a note to the page about CV (but not really sure what to say, since CVDevice doesn't exist yet)

  • Remove the CV operation/observable stuff for now (but could still be useful for internal developers?)

@antalszava antalszava self-requested a review January 28, 2020 13:39
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.

Left one comment about adding Identity to the set of observables

@codecov
Copy link

codecov bot commented Jan 28, 2020

Codecov Report

Merging #483 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #483   +/-   ##
=======================================
  Coverage   99.05%   99.05%           
=======================================
  Files          51       51           
  Lines        3923     3923           
=======================================
  Hits         3886     3886           
  Misses         37       37
Impacted Files Coverage Δ
pennylane/beta/__init__.py 100% <0%> (ø) ⬆️
pennylane/beta/plugins/__init__.py 100% <0%> (ø) ⬆️
pennylane/beta/plugins/expt_tensornet.py 97.68% <0%> (ø) ⬆️
pennylane/beta/plugins/expt_tensornet_tf.py 92.03% <0%> (ø) ⬆️
pennylane/qnodes/device_jacobian.py 90.9% <0%> (ø) ⬆️
pennylane/interfaces/tf.py 93.02% <0%> (ø) ⬆️
pennylane/qnodes/decorator.py 100% <0%> (ø) ⬆️
pennylane/qnodes/jacobian.py 100% <0%> (ø) ⬆️
pennylane/_device.py 99.27% <0%> (ø) ⬆️

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 6830356...734eef6. Read the comment docs.

Comment on lines 444 to 445
class is deprecated, and a new** ``CVDevice`` **class will be available in the next major
PennyLane release.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Better: will be available soon? I learnt to not promise things if not absolutely necessary :)

Copy link
Contributor

@mariaschuld mariaschuld left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, approved from my side.

doc/development/plugins.rst Outdated Show resolved Hide resolved
doc/development/plugins.rst Outdated Show resolved Hide resolved
@josh146 josh146 merged commit 2d8d5fe into master Jan 29, 2020
@josh146 josh146 deleted the actually-update-plugin-guide branch January 29, 2020 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devices 💻 Device or plugin API related documentation 📘 Documentation changes and updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants