[OpRefactor] Do not loop over all ops in tests #1895

mariaschuld · 2021-11-15T13:31:53Z

Context:

We want to delete the static num_params and num_wires attributes of operations, since in many cases these are ill-defined before instantiation. With this change, we cannot import operator classes, infer their arguments (which becomes less and less trivial anyways) and use them in tests, as is often done.

However, many of these tests should not loop automatically over operations anyways - the operations should be well-unit-tested, and outside logic should be tested with a few sample operations.

Description of the Change:

Rewrites or deletes tests where we loop blindly over operations.

Benefits:

Better test. Unblocks operator refactor.

Possible Drawbacks:

I may not have caught all cases, but I worked with the list of issues that arose from the PRs that this one is supposed to unblock!

Related GitHub Issues:

github-actions · 2021-11-15T13:32:10Z

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

A one-to-two sentence description of the change. You may include a small working example for new features.
A link back to this PR.
Your name (or GitHub username) in the contributors section.

mariaschuld · 2021-11-15T13:32:24Z

tests/tape/test_reversible.py

-        qml.DoubleExcitationPlus,
-        qml.DoubleExcitationMinus,
-        qml.OrbitalRotation,
-    }


This was funny: the logic above leaves analytic_qubit_ops with only 2-3 operations anyways :)

mariaschuld · 2021-11-15T13:33:01Z

tests/tape/test_reversible.py

-        qml.OrbitalRotation,
-    }
-
-    @pytest.mark.parametrize("obs", [qml.PauliX, qml.PauliY])


I only test with one obs, the other should work in the same way!

codecov · 2021-11-15T13:49:36Z

Codecov Report

Merging #1895 (468a47c) into master (590704b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1895   +/-   ##
=======================================
  Coverage   98.98%   98.98%           
=======================================
  Files         219      219           
  Lines       16512    16512           
=======================================
  Hits        16344    16344           
  Misses        168      168

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 590704b...468a47c. Read the comment docs.

mariaschuld · 2021-11-15T13:57:23Z

tests/devices/test_default_gaussian.py

@@ -788,57 +788,3 @@ def circuit():
        ):
            circuit()
        assert dev.shots == sum(shots)
-


Deleted this one, because we have a test_apply that should test the same thing on a lower level.

mariaschuld · 2021-11-15T14:43:49Z

tests/devices/test_default_gaussian.py

-            assert gaussian_dev._state[0] == pytest.approx(expected_out[0], abs=tol)
-            assert gaussian_dev._state[1] == pytest.approx(expected_out[1], abs=tol)
+        gaussian_dev = qml.device("default.gaussian", wires=1)
+


My thinking is this: if the apply function works, then the principle works. So I am trying it out with a constant dummy gate implementation that only transforms the covariance matrix.

Below I add more tests for gates that use forked logic in the apply function.

yes! If this is a unit test testing the apply method, we only should be checking that apply is doing as needed. There is no need to test every supported gate through apply.

mariaschuld · 2021-11-16T06:47:07Z

tests/beta/test_default_tensor.py

@@ -894,31 +894,21 @@ def circuit(*x):
            x = np.random.random([op.num_params])
            circuit(*x)

-    @pytest.mark.parametrize("observable", set(qml.ops.cv.obs))
-    def test_unsupported_observable_error(self, rep, observable):
+    def test_unsupported_observable_error(self, rep):


This is another bad pattern: to test an error message we looped over a big list of operations that produce the error, instead of one! (Same above)

so much cleaner!

mariaschuld · 2021-11-16T08:21:24Z

This is very strange, there are circuit drawer tests failing that I never touched...

josh146

I think this is one of my favourite PRs to review of all time. No suggestions! I love it!

josh146 · 2021-11-16T09:54:15Z

doc/releases/changelog-dev.md

+* Tests do not loop over automatically imported and instantiated operations any more, 
+  which was opaque and created unnecessarily many tests.
+  [(#1895)](https://github.com/PennyLaneAI/pennylane/pull/1895)


josh146 · 2021-11-16T09:54:54Z

tests/beta/test_default_tensor.py

        with pytest.raises(
            qml._device.DeviceError,
            match="not supported on device default.tensor",
        ):
-            x = np.random.random([op.num_params])
-            circuit(*x)
+            circuit()


this makes so much more sense 💯

josh146 · 2021-11-16T09:55:06Z

tests/beta/test_default_tensor.py

@@ -894,31 +894,21 @@ def circuit(*x):
            x = np.random.random([op.num_params])
            circuit(*x)

-    @pytest.mark.parametrize("observable", set(qml.ops.cv.obs))
-    def test_unsupported_observable_error(self, rep, observable):
+    def test_unsupported_observable_error(self, rep):


so much cleaner!

josh146 · 2021-11-16T09:55:51Z

tests/devices/test_default_gaussian.py

-            assert gaussian_dev._state[0] == pytest.approx(expected_out[0], abs=tol)
-            assert gaussian_dev._state[1] == pytest.approx(expected_out[1], abs=tol)
+        gaussian_dev = qml.device("default.gaussian", wires=1)
+


yes! If this is a unit test testing the apply method, we only should be checking that apply is doing as needed. There is no need to test every supported gate through apply.

josh146 · 2021-11-16T09:57:09Z

tests/tape/test_reversible.py

-        qml.DoubleExcitationPlus,
-        qml.DoubleExcitationMinus,
-        qml.OrbitalRotation,
-    }


first test done

4b26608

mariaschuld commented Nov 15, 2021

View reviewed changes

no 2, 3, 4

63e5ad3

mariaschuld commented Nov 15, 2021

View reviewed changes

another one

ee98698

mariaschuld commented Nov 15, 2021

View reviewed changes

mariaschuld added 2 commits November 16, 2021 08:44

another one

6b03c0e

another test

91e530f

mariaschuld commented Nov 16, 2021

View reviewed changes

mariaschuld added 4 commits November 16, 2021 09:06

final test rewritten

7557cc6

black

f20b9ed

add more gaussian device apply tests

41b8c16

Merge branch 'master' into rewrite-tests-that-loop-over-ops

d572d02

mariaschuld requested a review from josh146 November 16, 2021 07:28

josh146 approved these changes Nov 16, 2021

View reviewed changes

mariaschuld added 2 commits November 16, 2021 13:57

Merge branch 'master' into rewrite-tests-that-loop-over-ops

665ba54

Merge branch 'master' into rewrite-tests-that-loop-over-ops

468a47c

mariaschuld merged commit 534de0a into master Nov 16, 2021

mariaschuld deleted the rewrite-tests-that-loop-over-ops branch November 16, 2021 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OpRefactor] Do not loop over all ops in tests #1895

[OpRefactor] Do not loop over all ops in tests #1895

mariaschuld commented Nov 15, 2021 •

edited

github-actions bot commented Nov 15, 2021

mariaschuld Nov 15, 2021

josh146 Nov 16, 2021

mariaschuld Nov 15, 2021

codecov bot commented Nov 15, 2021 •

edited

mariaschuld Nov 15, 2021

mariaschuld Nov 15, 2021 •

edited

josh146 Nov 16, 2021

mariaschuld Nov 16, 2021 •

edited

josh146 Nov 16, 2021

mariaschuld commented Nov 16, 2021

josh146 left a comment

josh146 Nov 16, 2021

josh146 Nov 16, 2021

josh146 Nov 16, 2021

josh146 Nov 16, 2021

josh146 Nov 16, 2021

[OpRefactor] Do not loop over all ops in tests #1895

[OpRefactor] Do not loop over all ops in tests #1895

Conversation

mariaschuld commented Nov 15, 2021 • edited

github-actions bot commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

mariaschuld Nov 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariaschuld Nov 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariaschuld commented Nov 16, 2021

josh146 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

mariaschuld commented Nov 15, 2021 •

edited

codecov bot commented Nov 15, 2021 •

edited

mariaschuld Nov 15, 2021 •

edited

mariaschuld Nov 16, 2021 •

edited