Update plugin by removing `analytic` argument for device creation #93

antalszava · 2021-03-27T00:34:44Z

Updates the plugin to cope with removing the analytic keyword argument as per PennyLaneAI/pennylane#1079.

codecov · 2021-03-27T00:36:12Z

Codecov Report

Merging #93 (b89bf0a) into master (9a0305c) will not change coverage.
The diff coverage is 100.00%.

❗ Current head b89bf0a differs from pull request most recent head 8a6182a. Consider uploading reports for the commit 8a6182a to get more accurate results

@@           Coverage Diff           @@
##           master      #93   +/-   ##
=======================================
  Coverage   98.14%   98.14%           
=======================================
  Files           3        3           
  Lines          54       54           
=======================================
  Hits           53       53           
  Misses          1        1

Impacted Files	Coverage Δ
pennylane_lightning/_version.py	`100.00% <100.00%> (ø)`
pennylane_lightning/lightning_qubit.py	`97.95% <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 9a0305c...8a6182a. Read the comment docs.

josh146 · 2021-04-12T08:31:14Z

@thisac @trbromley just a reminder to review this one!

thisac

Looks good! Just one question: are the PL device tests updated as well, and should then the line pl-device-test --device lightning.qubit --analytic False --skip-ops --shots=20000 in build.yml be changed here?

thisac · 2021-04-12T13:53:42Z

requirements.txt

@@ -1,6 +1,6 @@
 flaky
 numpy
-pennylane>=0.12
+git+https://github.com/PennyLaneAI/pennylane.git


Do we want to wait until after release before merging this (and then update the requirements to the latest release instead)?

In the past we have, but we have so many plugins now, I think it is better to merge it as-is (and simply replace it with pennylane>=0.15 once it is on PyPI).

By merging it in now, we can:

Have its entry in the test matrix turn green,

Keep track of it breaking (again!) between now and release :)

trbromley

Thanks @antalszava! I had a few questions/comments, but otherwise looking good!

pennylane_lightning/lightning_qubit.py

trbromley · 2021-04-12T17:07:38Z

tests/test_apply.py

@@ -1135,7 +1134,7 @@ class TestTensorSample:

    def test_paulix_pauliy(self, theta, phi, varphi, monkeypatch, tol):
        """Test that a tensor product involving PauliX and PauliY works correctly"""
-        dev = qml.device("lightning.qubit", wires=3, shots=10000)
+        dev = qml.device("lightning.qubit", wires=3, shots=None)


Why does this one need to change?

Because we implicitly had analytic=True before

🤔 This one's still not clear to me - so before we had analytic=True and shots=10000. In the test, we call dev.sample() so it would have generated 10000 samples, yet these samples were not used in dev.probability() because we were in analytic mode?

So perhaps the test didn't need to set shots or call dev.sample()?

Would this just work if we had a parametrize over shots, setting [None, 10000], so we test both analytic and non-analytic cases?

Sure, changed this. Also removed the calls to dev.sample.

trbromley · 2021-04-12T17:08:25Z

tests/test_expval.py

@@ -30,7 +30,7 @@


 @pytest.mark.parametrize("theta, phi", list(zip(THETA, PHI)))
-@pytest.mark.parametrize("shots", [8192])
+@pytest.mark.parametrize("shots", [None])


Why do these ones need to change?

Same as above

Actually, do you agree that the shots fixture is not actively used in any of the tests below? 😕 Unless it's hidden somewhere in a fixture. So we could set to any value.

In which case, ideally we would remove the paramtrize and shots argument. But ok for this PR if we want to just change to None as you have done 👍

Yep, removed it

trbromley · 2021-04-12T17:09:19Z

tests/test_var.py

@@ -30,13 +30,13 @@


 @pytest.mark.parametrize("theta, phi", list(zip(THETA, PHI)))
-@pytest.mark.parametrize("shots", [8192])
+@pytest.mark.parametrize("shots", [None])


Do we need to parametrize over None (which is the default)?

No, but in this PR rather changed the analytic argument rather than adjusted how the testing is done

Co-authored-by: Tom Bromley <[email protected]>

…lightning into no_analytic_kwarg

antalszava · 2021-04-13T19:00:45Z

Thanks for the comments and the catches! Updated the PR

trbromley

Thanks @antalszava! Looks close to ✔️, I just had a few final questions.

trbromley · 2021-04-14T12:04:39Z

tests/test_apply.py

@@ -1135,7 +1134,7 @@ class TestTensorSample:

    def test_paulix_pauliy(self, theta, phi, varphi, monkeypatch, tol):
        """Test that a tensor product involving PauliX and PauliY works correctly"""
-        dev = qml.device("lightning.qubit", wires=3, shots=10000)
+        dev = qml.device("lightning.qubit", wires=3, shots=None)


🤔 This one's still not clear to me - so before we had analytic=True and shots=10000. In the test, we call dev.sample() so it would have generated 10000 samples, yet these samples were not used in dev.probability() because we were in analytic mode?

So perhaps the test didn't need to set shots or call dev.sample()?

Would this just work if we had a parametrize over shots, setting [None, 10000], so we test both analytic and non-analytic cases?

trbromley · 2021-04-14T12:12:46Z

tests/test_expval.py

@@ -30,7 +30,7 @@


 @pytest.mark.parametrize("theta, phi", list(zip(THETA, PHI)))
-@pytest.mark.parametrize("shots", [8192])
+@pytest.mark.parametrize("shots", [None])


Actually, do you agree that the shots fixture is not actively used in any of the tests below? 😕 Unless it's hidden somewhere in a fixture. So we could set to any value.

In which case, ideally we would remove the paramtrize and shots argument. But ok for this PR if we want to just change to None as you have done 👍

pennylane_lightning/lightning_qubit.py

trbromley

Thanks @antalszava for accommodating those suggestions! 💯

antalszava added 2 commits March 26, 2021 20:14

PL master

4febabf

changes

cc54300

antalszava added 2 commits March 26, 2021 20:42

PL master setup.py

40db376

adjust setup.py

149b656

antalszava requested review from trbromley and thisac April 1, 2021 12:58

thisac approved these changes Apr 12, 2021

View reviewed changes

trbromley suggested changes Apr 12, 2021

View reviewed changes

antalszava and others added 3 commits April 13, 2021 14:52

no analytic in build.yml

1d7577b

Update pennylane_lightning/lightning_qubit.py

652c6df

Co-authored-by: Tom Bromley <[email protected]>

Merge branch 'no_analytic_kwarg' of github.com:PennyLaneAI/pennylane-…

b920c2e

…lightning into no_analytic_kwarg

antalszava requested a review from trbromley April 13, 2021 19:00

trbromley reviewed Apr 14, 2021

View reviewed changes

pennylane_lightning/lightning_qubit.py Show resolved Hide resolved

antalszava added 4 commits April 14, 2021 08:57

no shots in tests

aaf9c3d

PL requires

0e37c37

shots change

eea2acd

shots=None and 10000 for test_apply

ade546e

antalszava requested a review from trbromley April 14, 2021 13:48

trbromley approved these changes Apr 14, 2021

View reviewed changes

antalszava added 3 commits April 14, 2021 10:02

changelog and version number

ea4c657

no extra changelog sections

b6d2e41

changelog

8a6182a

antalszava merged commit 0064c24 into master Apr 14, 2021

antalszava deleted the no_analytic_kwarg branch April 14, 2021 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update plugin by removing `analytic` argument for device creation #93

Update plugin by removing `analytic` argument for device creation #93

antalszava commented Mar 27, 2021 •

edited

Loading

codecov bot commented Mar 27, 2021 •

edited

Loading

josh146 commented Apr 12, 2021

thisac left a comment

thisac Apr 12, 2021

josh146 Apr 12, 2021

trbromley left a comment

trbromley Apr 12, 2021

antalszava Apr 13, 2021

trbromley Apr 14, 2021

antalszava Apr 14, 2021

trbromley Apr 12, 2021

antalszava Apr 13, 2021

trbromley Apr 14, 2021

antalszava Apr 14, 2021

trbromley Apr 12, 2021

antalszava Apr 13, 2021 •

edited

Loading

antalszava commented Apr 13, 2021

trbromley left a comment

trbromley Apr 14, 2021

trbromley Apr 14, 2021

trbromley left a comment

Update plugin by removing analytic argument for device creation #93

Update plugin by removing analytic argument for device creation #93

Conversation

antalszava commented Mar 27, 2021 • edited Loading

codecov bot commented Mar 27, 2021 • edited Loading

Codecov Report

josh146 commented Apr 12, 2021

thisac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

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 Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

antalszava commented Apr 13, 2021

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

trbromley left a comment

Choose a reason for hiding this comment

Update plugin by removing `analytic` argument for device creation #93

Update plugin by removing `analytic` argument for device creation #93

antalszava commented Mar 27, 2021 •

edited

Loading

codecov bot commented Mar 27, 2021 •

edited

Loading

antalszava Apr 13, 2021 •

edited

Loading