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

glassnotes · 2021-03-24T12:26:22Z

Fixes the plugin to work with new changes in PennyLane:

the analytic argument has been removed and replaced by shots=None where applicable
the qml._queueing.OperationRecorder has been replaced by qml.tape.OperationRecorder in tests where applicable

(I also ran black on the code, and it changed a bit more formatted than I expected so sorry the changes may be a bit buried in some files).

codecov · 2021-03-24T12:43:46Z

Codecov Report

Merging #130 (e9fc18e) into master (908eaec) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
- Coverage   99.30%   99.30%   -0.01%     
==========================================
  Files           7        7              
  Lines         289      286       -3     
==========================================
- Hits          287      284       -3     
  Misses          2        2

Impacted Files	Coverage Δ
pennylane_qiskit/aer.py	`100.00% <ø> (ø)`
pennylane_qiskit/basic_aer.py	`100.00% <ø> (ø)`
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 908eaec...e9fc18e. Read the comment docs.

pennylane_qiskit/qiskit_device.py

Co-authored-by: antalszava <[email protected]>

.github/workflows/docs.yml

This reverts commit e0d31de.

This reverts commit 92cb68e.

requirements.txt

antalszava · 2021-03-25T20:07:43Z

.github/workflows/tests.yml

-          pl-device-test --device=qiskit.aer --tb=short --skip-ops --analytic=True --device-kwargs backend=unitary_simulator
+          pl-device-test --device=qiskit.basicaer --tb=short --skip-ops --shots=20000 --device-kwargs backend=qasm_simulator
+          pl-device-test --device=qiskit.aer --tb=short --skip-ops --shots=20000 --device-kwargs backend=qasm_simulator
+          pl-device-test --device=qiskit.aer --tb=short --skip-ops --shots=None --device-kwargs backend=statevector_simulator


Failures in the device test suite originated from the fact that the device keyword arguments were ignored by PennyLane core. Therefore, even for backend=statevector_simulator we got qasm_simulator, though this time with shots=None, which were then changed to 1024 internally (HW warning branch).

requirements.txt

antalszava

Looks good to me! 💯 Thank you! 🙂 Coverage seems to be okay locally, not sure why codecov fails 🤔

Usually, using black, the source files would be formatted and tests are left intact, though those can also be formatted (black -l 100 pennylane_qiskit). If the tests are formatted, it's good to separate a change in logic and formatting. Left comments across the test files for places where I noticed a change, let me know if something was missed. 🙂

tests/conftest.py

antalszava · 2021-03-26T20:14:08Z

tests/test_apply.py

-@pytest.mark.parametrize("analytic", [True])
-@pytest.mark.parametrize("shots", [8192])
+
+@pytest.mark.parametrize("shots", [None])


Above 3 diffs were changes

antalszava · 2021-03-26T20:14:13Z

tests/test_apply.py

 @pytest.mark.usefixtures("skip_unitary")
 class TestNonAnalyticApply:
-    """Test application of PennyLane operations with the analytic attribute set to False."""
+    """Test application of PennyLane operations with non-analytic calculation."""


antalszava · 2021-03-26T20:15:19Z

tests/test_converter.py

+    @pytest.mark.parametrize("shots", [None])
    @pytest.mark.parametrize("theta,phi,varphi", list(zip(THETA, PHI, VARPHI)))
-    def test_gradient(self, theta, phi, varphi, analytic, tol):
+    def test_gradient(self, theta, phi, varphi, shots, tol):


antalszava · 2021-03-26T20:15:24Z

tests/test_converter.py

@@ -1001,7 +1021,7 @@ def test_gradient(self, theta, phi, varphi, analytic, tol):
        # convert to a PennyLane circuit
        qc_pl = qml.from_qiskit(qc)

-        dev = qml.device("default.qubit", wires=3, analytic=analytic)
+        dev = qml.device("default.qubit", wires=3, shots=shots)


antalszava · 2021-03-26T20:19:15Z

tests/test_qiskit_device.py


        with pytest.warns(UserWarning) as record:
-            dev = qml.device("qiskit.basicaer", backend=hardware_backend, wires=2, analytic=True)
+            dev = qml.device("qiskit.basicaer", backend=hardware_backend, wires=2, shots=None)


antalszava · 2021-03-26T20:19:21Z

tests/test_qiskit_device.py


-        dev = qml.device("qiskit.basicaer", backend=statevector_backend, wires=2, analytic=True)
+        dev = qml.device("qiskit.basicaer", backend=statevector_backend, wires=2, shots=None)


antalszava · 2021-03-26T20:19:29Z

tests/test_sample.py

@@ -15,7 +15,6 @@
 VARPHI = np.linspace(0.02, 1, 3)


-@pytest.mark.parametrize("analytic", [False])


antalszava · 2021-03-26T20:19:38Z

tests/test_var.py

@@ -16,8 +16,7 @@


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


antalszava · 2021-03-26T20:19:44Z

tests/test_var.py

@@ -74,8 +73,7 @@ def test_var_hermitian(self, theta, phi, device, shots, tol):


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


Co-authored-by: antalszava <[email protected]>

glassnotes · 2021-03-26T20:44:39Z

tests/conftest.py

        yield qml.Device()


 @pytest.fixture(scope="function")
 def recorder():
-    return qml._queuing.OperationRecorder()
+    return qml.tape.OperationRecorder()


glassnotes · 2021-03-26T20:44:50Z

tests/conftest.py

-def device(request, backend, shots, analytic):
-    if backend not in state_backends and analytic == True:
+def device(request, backend, shots):
+    if backend not in state_backends and shots is None:


glassnotes · 2021-03-26T20:45:21Z

tests/conftest.py

    def _device(n):
-        return request.param(wires=n, backend=statevector_backend, shots=shots, analytic=analytic)
+        return request.param(wires=n, backend=statevector_backend, shots=shots)


(This and the previous two are changed)

glassnotes · 2021-03-26T20:47:33Z

tests/test_integration.py

-        dev = qml.device(d[0], wires=1, backend=backend, shots=shots, analytic=analytic)
+    def test_one_qubit_circuit(self, shots, d, backend, tol):
+        """Integration test for the BasisState and Rot operations for non-analytic mode."""
+        dev = qml.device(d[0], wires=1, backend=backend, shots=shots)


glassnotes · 2021-03-26T20:50:28Z

Looks good to me! 100 Thank you! slightly_smiling_face Coverage seems to be okay locally, not sure why codecov fails thinking

Usually, using black, the source files would be formatted and tests are left intact, though those can also be formatted (black -l 100 pennylane_qiskit). If the tests are formatted, it's good to separate a change in logic and formatting. Left comments across the test files for places where I noticed a change, let me know if something was missed. slightly_smiling_face

Thanks @antalszava ! I didn't know the tests don't usually get blacked, sorry for the hassle! I marked a few other spots where there were changes. Can we override codecov in order to merge in?

pennylane_qiskit/qiskit_device.py

pennylane_qiskit/aer.py

pennylane_qiskit/basic_aer.py

josh146 · 2021-03-28T14:20:13Z

Thanks @antalszava ! I didn't know the tests don't usually get blacked, sorry for the hassle!

Note: this is just a historical accident that the tests don't get blacked/checked on GitHub. Partially to make the transition to blacking easier for everyone. While probably better to black the tests in their own PR, nice to have the tests finally blacked 😆

Co-authored-by: antalszava <[email protected]>

glassnotes · 2021-03-28T15:42:47Z

Thanks @antalszava ! I didn't know the tests don't usually get blacked, sorry for the hassle!

Note: this is just a historical accident that the tests don't get blacked/checked on GitHub. Partially to make the transition to blacking easier for everyone. While probably better to black the tests in their own PR, nice to have the tests finally blacked laughing

@josh146 I see, thanks for the context!

Regarding tests - could you please override codecov? (it's showing a diff of like -0.01%)

Olivia Di Matteo and others added 3 commits March 24, 2021 08:22

Remove analytic argument from all methods and tests. Black all code.

187498e

Remove analytic argument from workflow tests.

dcd9544

Update requirements.txt

3d9103e

glassnotes requested a review from thisac March 24, 2021 12:43

antalszava reviewed Mar 25, 2021

View reviewed changes

pennylane_qiskit/qiskit_device.py Outdated Show resolved Hide resolved

antalszava reviewed Mar 25, 2021

View reviewed changes

pennylane_qiskit/qiskit_device.py Outdated Show resolved Hide resolved

antalszava and others added 2 commits March 25, 2021 14:47

sphinx action from Josh

92cb68e

Apply suggestions from code review

6caf57d

Co-authored-by: antalszava <[email protected]>

antalszava reviewed Mar 25, 2021

View reviewed changes

.github/workflows/docs.yml Outdated Show resolved Hide resolved

antalszava added 4 commits March 25, 2021 14:54

Update .github/workflows/docs.yml

e0d31de

add UsageDetails directive

3a322eb

Revert "Update .github/workflows/docs.yml"

2aefab0

This reverts commit e0d31de.

Revert "sphinx action from Josh"

1030318

This reverts commit 92cb68e.

antalszava reviewed Mar 25, 2021

View reviewed changes

requirements.txt Show resolved Hide resolved

Update requirements.txt

ef6363a

antalszava reviewed Mar 25, 2021

View reviewed changes

antalszava mentioned this pull request Mar 25, 2021

Update device_kwargs PennyLaneAI/pennylane#1167

Merged

antalszava reviewed Mar 26, 2021

View reviewed changes

requirements.txt Outdated Show resolved Hide resolved

Update requirements.txt

e50fbf5

antalszava approved these changes Mar 26, 2021

View reviewed changes

Apply suggestions from code review

046a21a

Co-authored-by: antalszava <[email protected]>

glassnotes commented Mar 26, 2021

View reviewed changes

antalszava reviewed Mar 27, 2021

View reviewed changes

pennylane_qiskit/qiskit_device.py Outdated Show resolved Hide resolved

antalszava reviewed Mar 27, 2021

View reviewed changes

pennylane_qiskit/aer.py Outdated Show resolved Hide resolved

antalszava reviewed Mar 27, 2021

View reviewed changes

pennylane_qiskit/basic_aer.py Outdated Show resolved Hide resolved

Apply suggestions from code review

e9fc18e

Co-authored-by: antalszava <[email protected]>

josh146 merged commit ef50766 into master Mar 28, 2021

josh146 deleted the remove-analytic-argument branch March 28, 2021 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

glassnotes commented Mar 24, 2021

codecov bot commented Mar 24, 2021 •

edited

Loading

antalszava Mar 25, 2021

antalszava left a comment

antalszava Mar 26, 2021

antalszava Mar 26, 2021

antalszava Mar 26, 2021

antalszava Mar 26, 2021

antalszava Mar 26, 2021

antalszava Mar 26, 2021

antalszava Mar 26, 2021

antalszava Mar 26, 2021

antalszava Mar 26, 2021

glassnotes Mar 26, 2021

glassnotes Mar 26, 2021

glassnotes Mar 26, 2021

glassnotes Mar 26, 2021

glassnotes commented Mar 26, 2021

josh146 commented Mar 28, 2021

glassnotes commented Mar 28, 2021


		dev = qml.device("qiskit.basicaer", backend=statevector_backend, wires=2, analytic=True)
		dev = qml.device("qiskit.basicaer", backend=statevector_backend, wires=2, shots=None)

		@@ -15,7 +15,6 @@
		VARPHI = np.linspace(0.02, 1, 3)


		@pytest.mark.parametrize("analytic", [False])

Update plugin by removing analytic argument for device creation #130

Update plugin by removing analytic argument for device creation #130

Conversation

glassnotes commented Mar 24, 2021

codecov bot commented Mar 24, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

antalszava 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

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

glassnotes commented Mar 26, 2021

josh146 commented Mar 28, 2021

glassnotes commented Mar 28, 2021

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

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

codecov bot commented Mar 24, 2021 •

edited

Loading