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

Rename plugins folder #726

Merged
merged 22 commits into from
Aug 5, 2020
Merged

Rename plugins folder #726

merged 22 commits into from
Aug 5, 2020

Conversation

mariaschuld
Copy link
Contributor

@mariaschuld mariaschuld commented Jul 29, 2020

This PR renames the plugins folders in core PennyLane to "devices", to avoid confusion for developers (since what they contain are devices). It also updates some outdated docstrings here and there.

It is part of an overall effort to clarify our device ecosystem for users and developers.

NOTE:

  1. After merging this PR, every developer would need to pip install -e . How can we communicate this to external developers?
  2. At some later stage, it would be great to also update the "entry_points" string 'pennylane.plugins', which is used to load all installed devices. However, this requires changes to the setup file of all devices, and since only very few developers know about this string in the first place, this PR just adds a TODO.

pennylane/__init__.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 29, 2020

Codecov Report

Merging #726 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #726   +/-   ##
=======================================
  Coverage   95.62%   95.62%           
=======================================
  Files         111      111           
  Lines        6944     6944           
=======================================
  Hits         6640     6640           
  Misses        304      304           
Impacted Files Coverage Δ
pennylane/__init__.py 64.19% <ø> (ø)
pennylane/beta/devices/__init__.py 100.00% <ø> (ø)
pennylane/beta/devices/numpy_ops.py 100.00% <ø> (ø)
pennylane/devices/__init__.py 100.00% <ø> (ø)
pennylane/devices/autograd_ops.py 97.29% <ø> (ø)
pennylane/devices/default_gaussian.py 98.93% <ø> (ø)
pennylane/devices/default_qubit.py 98.31% <ø> (ø)
pennylane/devices/default_qubit_tf.py 86.95% <ø> (ø)
pennylane/devices/tf_ops.py 97.72% <ø> (ø)
pennylane/qnodes/jacobian.py 97.27% <ø> (ø)
... and 3 more

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 c2d27cc...fd54985. Read the comment docs.

setup.py Show resolved Hide resolved
@mariaschuld mariaschuld changed the title [WIP] Rename plugins folder Rename plugins folder Jul 29, 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.

Looks good to me @mariaschuld! Thanks!

  • One important spot would be updating the Makefile too (here and once later):

    PLUGIN_TESTRUNNER := -m pytest pennylane/plugins/tests --tb=native --no-flaky-report

  • Going for renaming the entry points could occur right before release for example.

@mariaschuld
Copy link
Contributor Author

  • Going for renaming the entry points could occur right before release for example.

Great, thanks for pointing out the MakeFile!

@mariaschuld
Copy link
Contributor Author

@josh146 I think the device tests are failing because CI needs to change the file path with this PR?

@josh146
Copy link
Member

josh146 commented Aug 4, 2020

@josh146 I think the device tests are failing because CI needs to change the file path with this PR?

@mariaschuld yep! You'll need to update the corresponding .github/workflow file to use the new directory structure when running the device tests

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @mariaschuld!

You mentioned

It is part of an overall effort to clarify our device ecosystem for users and developers.

I just wanted to ask roughly what the next steps will be? I noticed "plugin" is mentioned a lot still in the docs, could there be any occurrences there that warrant changing to "device"? Maybe that is for a follow up PR.

Makefile Show resolved Hide resolved
@@ -64,6 +64,10 @@

<h3>Improvements</h3>

* The `pennylane.plugins` and `pennylane.beta.plugins` folders have been renamed to
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to update "Adds a device test suite, located at pennylane/plugins/tests"? This is for the current release so think it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also noticed that previously there are some links (line 140 and 143) that might be nice to update - I don't think we should change everything in the history, but it would be good for links to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be better to do in separate PR? I am trying to be more disciplined with such things :)

codecov.yml Outdated Show resolved Hide resolved
pennylane/devices/default_gaussian.py Outdated Show resolved Hide resolved
pennylane/devices/default_qubit.py Outdated Show resolved Hide resolved
.github/CHANGELOG.md Outdated Show resolved Hide resolved
codecov.yml Outdated Show resolved Hide resolved
@@ -1,7 +1,7 @@
qml.plugins
qml.devices
Copy link
Member

Choose a reason for hiding this comment

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

I know it's a bit late to point this out, but I suddenly wonder if we will be introducing confusion between

  • qml.device
  • qml.Device
  • qml.devices

I suppose only option is to be really clear in the docs

pennylane/devices/__init__.py Show resolved Hide resolved
pennylane/devices/default_qubit.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@mariaschuld
Copy link
Contributor Author

I just wanted to ask roughly what the next steps will be? I noticed "plugin" is mentioned a lot still in the docs, could there be any occurrences there that warrant changing to "device"? Maybe that is for a follow up PR.

The next steps would be more about capabilities of devices that we need to standarise: Some don't have the attribute analytic, which we then also want to rename, and the capabilities dictionary must be updated to allow us to automatically showcase device capabilities on the webpage...But I'll put "docs" on the todo list :)

@josh146 josh146 merged commit 378467c into master Aug 5, 2020
@josh146 josh146 deleted the rename_plugins_folder branch August 5, 2020 16:57
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.

None yet

4 participants