Test for comparing different devices with the default device #897

alejomonbar · 2020-11-11T19:33:41Z

Before submitting

Please complete the following checklist when submitting a PR:

All new features must include a unit test.
If you've fixed a bug or added code that should be tested, add a test to the
test directory!
All new functions and code must be clearly commented and documented.
If you do make documentation changes, make sure that the docs build and
render correctly by running make docs.
Ensure that the test suite passes, by running make test.
Add a new entry to the .github/CHANGELOG.md file, summarizing the
change, and including a link back to the PR.
The PennyLane source code conforms to
PEP8 standards.
We check all of our code against Pylint.
To lint modified files, simply pip install pylint, and then
run pylint pennylane/path/to/file.py.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.

Context:
The solution to Issue #848
Description of the Change:
New file to test if a device different from the default device gives the same results that the default device.
Benefits:

Possible Drawbacks:

Related GitHub Issues:

Updating my master branch

codecov · 2020-11-11T19:38:44Z

Codecov Report

Merging #897 (4883be5) into master (ce7c1de) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #897   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files         147      147           
  Lines       10127    10127           
=======================================
  Hits         9910     9910           
  Misses        217      217

Impacted Files	Coverage Δ
qchem/pennylane_qchem/qchem/structure.py	`96.66% <ø> (ø)`
pennylane/templates/utils.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 ce7c1de...4883be5. Read the comment docs.

trbromley · 2020-11-11T21:59:14Z

Thanks @alejomonbar this looks great! We'll take a look asap!

antalszava · 2020-11-12T02:24:12Z

pennylane/devices/tests/test_compare_default_qubit.py

+            pytest.skip("Skipped because device does not support the Hermitian observable.")
+
+        if dev.name == dev_def.name:
+            pytest.skip("Device is the default.qubit.")


Suggested change

pytest.skip("Device is the default.qubit.")

pytest.skip("Device is default.qubit.")

antalszava · 2020-11-12T02:25:08Z

pennylane/devices/tests/test_compare_default_qubit.py

+        assert np.allclose(grad(theta, phi), grad_def(theta, phi), atol=tol(dev.analytic))
+
+    def test_pauliz_expectation(self, device, tol):
+        """Test that PauliZ expectation value is correct"""


Suggested change

"""Test that PauliZ expectation value is correct"""

"""Test that the PauliZ expectation value is correct"""

antalszava · 2020-11-12T02:25:29Z

pennylane/devices/tests/test_compare_default_qubit.py

+        dev = device(n_wires)
+        dev_def = qml.device("default.qubit", wires=n_wires)
+        if dev.name == dev_def.name:
+            pytest.skip("Device is the default.qubit.")


Suggested change

pytest.skip("Device is the default.qubit.")

pytest.skip("Device is default.qubit.")

antalszava · 2020-11-12T02:25:40Z

pennylane/devices/tests/test_compare_default_qubit.py

+        dev = device(n_wires)
+        dev_def = qml.device("default.qubit", wires=n_wires)
+        if dev.name == dev_def.name:
+            pytest.skip("Device is the default.qubit.")


Suggested change

pytest.skip("Device is the default.qubit.")

pytest.skip("Device is default.qubit.")

antalszava · 2020-11-12T02:25:59Z

pennylane/devices/tests/test_compare_default_qubit.py

+        assert np.allclose(grad(theta, phi), grad_def(theta, phi), atol=tol(dev.analytic))
+
+    def test_random_circuit(self, device, tol):
+        """Test that random expectation value is correct"""


Suggested change

"""Test that random expectation value is correct"""

"""Test that the random expectation value is correct"""

antalszava · 2020-11-12T02:29:07Z

pennylane/devices/tests/test_compare_default_qubit.py

+        assert np.allclose(qnode(theta, phi), qnode_def(theta, phi), atol=tol(dev.analytic))
+        assert np.allclose(grad(theta, phi), grad_def(theta, phi), atol=tol(dev.analytic))
+
+    def test_pauliz_expectation(self, device, tol):


(Minor suggestion): could be worth including in the name, that the test targets devices run in analytic mode.

Suggested change

def test_pauliz_expectation(self, device, tol):

def test_pauliz_expectation_analytic(self, device, tol):

pennylane/devices/tests/test_compare_default_qubit.py

antalszava

Hi @alejomonbar, had a look at the addition, this is looking really great and useful! 🙂 🎊

Left a couple of comments/suggestions. One main change that would be nice to have is the ability to skip tests that use tensor observables.

trbromley

Thanks @alejomonbar, looking good so far! I've left a few comments. Don't forget to update the changelog. Cheers!

trbromley · 2020-11-12T19:30:12Z

pennylane/devices/tests/test_compare_default_qubit.py

+
+@flaky(max_runs=10)
+class TestQubit:
+    def test_easy_circuit(self, device, tol):


Suggested change

def test_easy_circuit(self, device, tol):

def test_hermitian_expectation(self, device, tol):

pennylane/devices/tests/test_compare_default_qubit.py

trbromley · 2020-11-12T19:36:49Z

pennylane/devices/tests/test_compare_default_qubit.py

+
+        def circuit(weights):
+            RandomLayers(weights, wires=range(n_wires))
+            return qml.expval(qml.PauliZ(wires=0) @ qml.PauliX(wires=1))


It would be cool to also have a pytest parametrization over return types that apply to observables, e.g.

@pytest.mark.parametrize("ret", [qml.expval, qml.sample, qml.var]) def test_random_circuit(self, device, tol, ret): ... return ret(qml.PauliZ(wires=0) @ qml.PauliX(wires=1))

I not completely sure but I think I cannot use qml.samples with qml.grad. Let me know if I can set this test with qml.samples. I implemented the other two in this random circuit. By the way, thank you for the feedback.

@alejomonbar that's correct, qml.sample does not currently support gradients. Feel free to remove it from the parametrization!

pennylane/devices/tests/test_compare_default_qubit.py

…e team

alejomonbar · 2020-11-16T14:03:40Z

@josh146 @trbromley @antalszava Thank you for your suggestions, I made some changes. I'm not sure if I did it correctly. I'm new updating repositories on GitHub, I used git push -f. Here is the file with the updates baf7ded

co9olguy · 2020-11-17T15:24:09Z

Thanks @alejomonbar for contributing!

As a tip related to best practices, try not to use the -f option when pushing. This will do a "force push", which (potentially) overwrites the existing version control history. In this case, I don't think it's done any harm, but if other users have been working on the same remote branch, it could cause their version control history to break

alejomonbar · 2020-11-30T20:28:41Z

@josh146 @antalszava does this solution works or should I do something else? By the way, I used Black to reformat the new file but it does not pass the test. Also, I included a function that randomly select gates from a set of gates for the last test.

co9olguy · 2020-11-30T22:19:28Z

Thanks @alejomonbar. Did you run black with a line-limit of 100 characters? (our default is different than black's default)
i.e., use black -l 100 on the file test_compare_default_qubit.py

antalszava · 2020-12-01T00:43:36Z

pennylane/devices/tests/test_compare_default_qubit.py

+from pennylane import numpy as np
+import pytest
+from flaky import flaky
+from pennylane.templates.layers import RandomLayers


These codefactor warnings could be resolved by running isort on this file. isort can be installed with pip and it sorts the imports in the expected order.

pennylane/devices/tests/test_compare_default_qubit.py

antalszava

Hi @alejomonbar, thank you so much for the adjustments! Overall the addition looks good to me. 🎊 💯

The last thing that remains would be to update the imports and format using black (let us know if that doesn't come along). After that, this would be merge-ready from my side.

josh146 · 2020-12-01T06:42:54Z

Hi @alejomonbar, I just pushed a fix that will hopefully fix the CI issue! With respect to black, are you using the latest version of Black?

On the CI check, we are using the following version:

$ black --version
black, version 20.8b1

alejomonbar · 2020-12-01T16:07:28Z

Hi @alejomonbar, I just pushed a fix that will hopefully fix the CI issue! With respect to black, are you using the latest version of Black?

On the CI check, we are using the following version:
$ black --version
black, version 20.8b1

Hi @josh146.
Yes, that's the version.

co9olguy · 2020-12-03T17:52:10Z

Looks like all tests are now passing! 💪

co9olguy · 2020-12-03T17:52:33Z

@antalszava @josh146 @trbromley where do we currently stand in the code review process?

trbromley · 2020-12-03T20:29:40Z

Thanks @alejomonbar for the progress! I'm looking forward to take another look at this, hoping to do so tomorrow morning.

antalszava · 2020-12-03T23:39:09Z

As the formatting is completed now, this one is good to go from my side. 💯

trbromley · 2020-12-04T19:45:14Z

I will probably not get a chance to review this until early next week. If it's good to merge, don't let me be a blocker 👍

trbromley

Thanks @alejomonbar, some really nice tests! I have a few more suggestions, but this seems pretty close to merging! Thanks for the contribution! 💯

pennylane/devices/tests/test_compare_default_qubit.py

trbromley · 2020-12-07T21:58:38Z

pennylane/devices/tests/test_compare_default_qubit.py

+import random
+
+import pytest
+from flaky import flaky
+
+import pennylane as qml
+
+# pylint: disable=no-self-use
+from pennylane import numpy as np
+from pennylane.templates.layers import RandomLayers


Suggested change

import random

import pytest

from flaky import flaky

import pennylane as qml

# pylint: disable=no-self-use

from pennylane import numpy as np

from pennylane.templates.layers import RandomLayers

import random

# pylint: disable=no-self-use

import numpy as np

import pytest

from flaky import flaky

import pennylane as qml

from pennylane.templates.layers import RandomLayers

^ Main change was that I believe we don't need from pennylane import numpy as np and instead import numpy as np is sufficient. Then the rest is a run of isort.

@trbromley adding from pennylane import numpy as np was a suggestion from my part previously (see #897 (comment)). Maybe we could leave a comment here to make it explicit, e.g., (# Importing NumPy from PennyLane to test the environment demonstrations use or something similar).

Thanks @antalszava! Sure I'm potentially happy to have from pennylane import numpy as np (although would be good to add a no-member to the pylint disable).

What do you mean about environment demonstrations? As in the demos here?

Yep, some demos (e.g., Variational classifier, Quanvolutional Neural Networks) have this import and use arrays to represent data. When processing user questions it turned out that the difference between using a pennylane.numpy.tensor instead of the original np.array can result in errors due to bugs.

Perhaps the word environment is not the best to use here, could also go with # Importing NumPy from PennyLane to use similar imports as in QML demonstrations or something better.

pennylane/devices/tests/test_compare_default_qubit.py

trbromley · 2020-12-07T22:41:15Z

pennylane/devices/tests/test_compare_default_qubit.py

+        gates = [
+            qml.PauliX,
+            qml.PauliY,
+            qml.PauliZ,
+            qml.S,
+            qml.T,
+            qml.RX,
+            qml.RY,
+            qml.RZ,
+            qml.Hadamard,
+            qml.Rot,
+            qml.CRot,
+            qml.Toffoli,
+            qml.SWAP,
+            qml.CSWAP,
+            qml.U1,
+            qml.U2,
+            qml.U3,
+            qml.CRX,
+            qml.CRY,
+            qml.CRZ,
+        ]


@antalszava @josh146 is there a way in PL to automate the creation of this list, so that it goes over all defined ops?

Could go with [getattr(qml, op) for op in qml.ops.qubit.__all__]. This will dynamically pull every operation that was defined in qml.ops.qubit.__all__ (that is care needs to be taken if some operations are meant to be explicitly excluded).

pennylane/devices/tests/test_compare_default_qubit.py

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

trbromley

Thanks @alejomonbar!

pennylane/devices/tests/test_compare_default_qubit.py

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

alejomonbar · 2020-12-08T22:33:25Z

Thanks @alejomonbar!

Thank you to you all for helping me through this process. Eager to continue learning and helping the Pennylane community.
@trbromley @josh146 @antalszava

trbromley · 2020-12-09T12:42:50Z

Thanks @alejomonbar, one last thing I remembered - don't forget to update the CHANGELOG.md with a brief summary of this contribution as well as adding your name as a contributor.

We're very keen for additional contributions, and have been recently posting suggestions for what to work on in our issues page.

alejomonbar · 2020-12-09T14:04:24Z

Thanks @alejomonbar, one last thing I remembered - don't forget to update the CHANGELOG.md with a brief summary of this contribution as well as adding your name as a contributor.

We're very keen for additional contributions, and have been recently posting suggestions for what to work on in our issues page.

I'm not completely sure about how to edit the CHANGELOG.md. In # Release 0.14.0-dev (development release) should I add it as Improvements ?

trbromley · 2020-12-09T14:16:05Z

Thanks @alejomonbar, one last thing I remembered - don't forget to update the CHANGELOG.md with a brief summary of this contribution as well as adding your name as a contributor.
We're very keen for additional contributions, and have been recently posting suggestions for what to work on in our issues page.

I'm not completely sure about how to edit the CHANGELOG.md. In # Release 0.14.0-dev (development release) should I add it as Improvements ?

Yes, you can add a few lines in the improvements of 0.14.0-dev. This should follow the established style, with a link to the PR at the end.

antalszava · 2020-12-09T15:29:00Z

🌟 🎊

trbromley · 2020-12-09T16:19:14Z

.github/CHANGELOG.md

+* A new test series, pennylane/devices/tests/test_compare_default_qubit.py, has been added, allowing to test if
+  a chosen device gives the same result as the default device. Three tests are added `test_hermitian_expectation`,
+  `test_pauliz_expectation_analytic`, and `test_random_circuit`.
+  [(#848)](https://github.com/PennyLaneAI/pennylane/pull/848)


(we may want to update to 897, better to link to the PR than the issue)

alejomonbar added 2 commits October 20, 2020 11:23

Merge pull request #1 from PennyLaneAI/master

b2e6eba

Updating my master branch

Test for comparing different devices with the default device

729ffc1

trbromley requested review from trbromley, antalszava and thisac November 11, 2020 21:58

antalszava reviewed Nov 12, 2020

View reviewed changes

pennylane/devices/tests/test_compare_default_qubit.py Show resolved Hide resolved

antalszava reviewed Nov 12, 2020

View reviewed changes

pennylane/devices/tests/test_compare_default_qubit.py Show resolved Hide resolved

antalszava reviewed Nov 12, 2020

View reviewed changes

pennylane/devices/tests/test_compare_default_qubit.py Outdated Show resolved Hide resolved

antalszava reviewed Nov 12, 2020

View reviewed changes

pennylane/devices/tests/test_compare_default_qubit.py Outdated Show resolved Hide resolved

antalszava suggested changes Nov 12, 2020

View reviewed changes

trbromley reviewed Nov 12, 2020

View reviewed changes

Update test_compare_default_qubit.py with the suggestions of Pennylan…

baf7ded

…e team

alejomonbar force-pushed the fix848 branch from 73b4c19 to baf7ded Compare November 16, 2020 13:46

Merge branch 'master' into fix848

b42625f

Merge branch 'master' into fix848

6689b71

antalszava reviewed Dec 1, 2020

View reviewed changes

pennylane/devices/tests/test_compare_default_qubit.py Show resolved Hide resolved

antalszava approved these changes Dec 1, 2020

View reviewed changes

fix

72d6f98

josh146 requested a review from trbromley December 1, 2020 06:43

josh146 added the review-ready 👌 PRs which are ready for review by someone from the core team. label Dec 1, 2020

Merge branch 'master' into fix848

4111260

trbromley reviewed Dec 7, 2020

View reviewed changes

pennylane/devices/tests/test_compare_default_qubit.py Outdated Show resolved Hide resolved

trbromley reviewed Dec 7, 2020

View reviewed changes

pennylane/devices/tests/test_compare_default_qubit.py Outdated Show resolved Hide resolved

alejomonbar and others added 2 commits December 8, 2020 08:01

Apply suggestions from code review

4a05765

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

Update pennylane/devices/tests/test_compare_default_qubit.py

1b05763

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

trbromley approved these changes Dec 8, 2020

View reviewed changes

pennylane/devices/tests/test_compare_default_qubit.py Outdated Show resolved Hide resolved

pennylane/devices/tests/test_compare_default_qubit.py Outdated Show resolved Hide resolved

Apply suggestions from code review

1a51746

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

Merge branch 'master' into fix848

86e30ed

Merge branch 'master' into fix848

0a0df24

Update CHANGELOG.md

4883be5

antalszava merged commit e1fb824 into PennyLaneAI:master Dec 9, 2020

trbromley mentioned this pull request Dec 9, 2020

Add comparison tests to device test suite #848

Closed

trbromley reviewed Dec 9, 2020

View reviewed changes

antalszava mentioned this pull request Dec 9, 2020

Correct PR number in changelog #960

Merged

	pytest.skip("Device is the default.qubit.")
	pytest.skip("Device is default.qubit.")

	"""Test that PauliZ expectation value is correct"""
	"""Test that the PauliZ expectation value is correct"""

	"""Test that random expectation value is correct"""
	"""Test that the random expectation value is correct"""

	def test_pauliz_expectation(self, device, tol):
	def test_pauliz_expectation_analytic(self, device, tol):

	def test_easy_circuit(self, device, tol):
	def test_hermitian_expectation(self, device, tol):

Test for comparing different devices with the default device #897

Test for comparing different devices with the default device #897

Conversation

alejomonbar commented Nov 11, 2020

Before submitting

codecov bot commented Nov 11, 2020 • edited Loading

Codecov Report

trbromley commented Nov 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava left a comment

Choose a reason for hiding this comment

trbromley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejomonbar commented Nov 16, 2020

co9olguy commented Nov 17, 2020

alejomonbar commented Nov 30, 2020

co9olguy commented Nov 30, 2020

Choose a reason for hiding this comment

antalszava left a comment

Choose a reason for hiding this comment

josh146 commented Dec 1, 2020

alejomonbar commented Dec 1, 2020

co9olguy commented Dec 3, 2020

co9olguy commented Dec 3, 2020 • edited Loading

trbromley commented Dec 3, 2020

antalszava commented Dec 3, 2020

trbromley commented Dec 4, 2020

trbromley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

trbromley left a comment

Choose a reason for hiding this comment

alejomonbar commented Dec 8, 2020

trbromley commented Dec 9, 2020

alejomonbar commented Dec 9, 2020

trbromley commented Dec 9, 2020

antalszava commented Dec 9, 2020

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2020 •

edited

Loading

co9olguy commented Dec 3, 2020 •

edited

Loading

antalszava Dec 7, 2020 •

edited

Loading