[OpRefactor] Remove par_domain attribute #1907

mariaschuld · 2021-11-17T06:31:30Z

The par_domain attribute of the operator class is an anachronism that is removed everywhere.

Luckily, this only affected tests. The only exception is a sanity check at the creation of operations, comparing the parameter domain to the grad method. However, one could argue that such a check is an unnecessary slowdown anyways, and should rather be covered by tests.

…neAI/pennylane into rewrite-tests-that-loop-over-ops

…m_params_dynamically

…ennylane into set_num_params_dynamically

codecov · 2021-11-17T08:50:34Z

Codecov Report

Merging #1907 (5d56bc0) into master (c3f4d04) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1907      +/-   ##
==========================================
- Coverage   98.85%   98.84%   -0.02%     
==========================================
  Files         220      220              
  Lines       16972    16843     -129     
==========================================
- Hits        16778    16648     -130     
- Misses        194      195       +1

Impacted Files	Coverage Δ
pennylane/ops/__init__.py	`100.00% <ø> (ø)`
pennylane/ops/channel.py	`100.00% <ø> (ø)`
pennylane/ops/cv.py	`100.00% <ø> (ø)`
pennylane/ops/qubit/arithmetic_ops.py	`100.00% <ø> (ø)`
pennylane/ops/qubit/hamiltonian.py	`99.04% <ø> (-0.01%)`	⬇️
pennylane/ops/qubit/matrix_ops.py	`99.16% <ø> (-0.03%)`	⬇️
pennylane/ops/qubit/non_parametric_ops.py	`97.30% <ø> (-0.10%)`	⬇️
pennylane/ops/qubit/observables.py	`100.00% <ø> (ø)`
pennylane/ops/qubit/parametric_ops.py	`99.82% <ø> (-0.01%)`	⬇️
pennylane/ops/qubit/qchem_ops.py	`100.00% <ø> (ø)`
... and 34 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 c3f4d04...5d56bc0. Read the comment docs.

mariaschuld · 2021-11-17T11:46:49Z

pennylane/operation.py

@@ -583,9 +570,7 @@ class Operation(Operator):
    As with :class:`~.Operator`, the following class attributes must be
    defined for all operations:

-    * :attr:`~.Operator.num_params`


Sorry, I forgot to remove these in an earlier PR

mariaschuld · 2021-11-17T11:47:19Z

pennylane/operation.py

-                None,
-                "F",
-            ), "Operations that depend on arrays containing free variables may only be differentiated using the F method."
-


This is deleted for good, since we cannot do this verification any more - we have to rely on the developers to do things right!

mariaschuld · 2021-11-17T11:48:04Z

tests/devices/test_default_qubit.py

-    "Convert par into a list of parameters that op expects."
-    if op.par_domain == "A":
-        return [np.diag([x, 1]) for x in par]
-    return par


This and the same below was made redundant by a previous PR anyways!

mariaschuld · 2021-11-17T11:48:24Z

tests/test_operation.py

@@ -115,57 +110,13 @@ class DummyOp(qml.operation.CVOperation):
        ):
            DummyOp(0.5, wires=[0, 1])

-    def test_grad_method_with_integer_params(self):


Since the check above is removed, these don't make sense any more

mariaschuld · 2021-11-17T11:49:10Z

tests/test_operation.py

-
-class TestObservableInstantiation:
-    """Test that wires are specified when a qml.operation.Observable is instantiated"""
-


Another thing I snuck in: removing this merges the single test function with the above test class TestObservableConstruction which is kind of the same

mariaschuld · 2021-11-17T11:50:03Z

tests/test_quantum_gradients.py

@@ -279,48 +279,36 @@ def circuit(x, y):
        assert grad_A == pytest.approx(grad_F, abs=tol)
        assert grad_A2 == pytest.approx(grad_F, abs=tol)

-    @pytest.mark.parametrize("name", qml.ops._cv__ops__)
-    def test_CVOperation_with_heisenberg_and_no_params(self, name, gaussian_dev, tol):
-        """An integration test for CV gates that support analytic differentiation


I realised that the if statement in the test reduces all CV ops loaded to InterfermoterUnitary and I make the test more explicit.

This is part of a pattern we enforce in a previous PR: do not automatically import and instantiate gates!

Useful to know, thanks!

mariaschuld · 2021-11-17T11:51:24Z

codecov complains about a new untested NotImplementedError which we generally don't test - I'll overwrite

anthayes92

Thanks @mariaschuld, liberating to see code being removed! Main question is whether num_params should be removed in a few places? It seems it's no longer needed for the Operator class and it's child classes. But otherwise good to go!

anthayes92 · 2021-11-17T15:25:18Z

pennylane/ops/qubit/state_preparation.py

@@ -146,7 +144,6 @@ def circuit():
    """
    num_params = 1


num_params was removed from operation.py, should it be removed here too?

anthayes92 · 2021-11-17T15:28:13Z

tests/beta/test_beta_qnode.py

@@ -932,7 +932,6 @@ def test_device_expansion(self, diff_method, mode, mocker):
        class UnsupportedOp(qml.operation.Operation):
            num_wires = 1
            num_params = 1


anthayes92 · 2021-11-17T15:28:46Z

pennylane/transforms/control.py

@@ -87,7 +87,6 @@ class ControlledOperation(Operation):
        control_wires: A wire or set of wires.
    """

-    par_domain = "A"
    num_wires = AnyWires
    num_params = property(lambda self: self.subtape.num_params)


Again, should num_params be removed?

anthayes92 · 2021-11-17T15:28:50Z

tests/beta/test_beta_qnode.py

@@ -965,7 +964,6 @@ def test_no_gradient_expansion(self, mocker):
        class UnsupportedOp(qml.operation.Operation):
            num_wires = 1
            num_params = 1


anthayes92 · 2021-11-17T15:30:45Z

tests/devices/test_default_qubit.py

@@ -2283,7 +2276,6 @@ class TestSGate(qml.operation.Operation):
            matrix = np.array([[0, 1], [1, 0]])
            num_params = 0


There are also a few num_params still existing in the tests.

anthayes92 · 2021-11-17T15:38:44Z

tests/test_quantum_gradients.py

@@ -279,48 +279,36 @@ def circuit(x, y):
        assert grad_A == pytest.approx(grad_F, abs=tol)
        assert grad_A2 == pytest.approx(grad_F, abs=tol)

-    @pytest.mark.parametrize("name", qml.ops._cv__ops__)
-    def test_CVOperation_with_heisenberg_and_no_params(self, name, gaussian_dev, tol):
-        """An integration test for CV gates that support analytic differentiation


Useful to know, thanks!

mariaschuld · 2021-11-17T17:21:28Z

@anthayes92 you are so right! Since this is kind of a different topic, let me open another PR for the num_params. I don't know how I missed them in the previous one.

mariaschuld added 29 commits November 3, 2021 10:23

turn all static attributes to dynamic ones, or delete them altogether

4c570c1

fix and clean tests

7ee433e

fix more tests

cea9379

first test done

4b26608

no 2, 3, 4

63e5ad3

another one

ee98698

another one

6b03c0e

another test

91e530f

final test rewritten

7557cc6

black

f20b9ed

add more gaussian device apply tests

41b8c16

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

d572d02

fix conflicts

e9b18ad

Merge branch 'rewrite-tests-that-loop-over-ops' of github.com:PennyLa…

0c59d04

…neAI/pennylane into rewrite-tests-that-loop-over-ops

Merge branch 'master' of github.com:PennyLaneAI/pennylane into set_nu…

959a9da

…m_params_dynamically

resolve conflicts

bbf8e7d

changelog

0c78a77

fix codefactor

e58f177

backup

86c1c33

adapt new test

fef3090

fix some more tests

cb2953e

fix some more tests

e1028bf

Merge branch 'master' into set_num_params_dynamically

8d44ce8

small fix

75d7a34

Merge branch 'set_num_params_dynamically' of github.com:PennyLaneAI/p…

c387023

…ennylane into set_num_params_dynamically

remove par_domain everywhere

04da14f

increase coverage

da84233

Merge branch 'set_num_params_dynamically' into remove-par-domain

87e066f

update PR number

3071c86

josh146 changed the base branch from master to set_num_params_dynamically November 17, 2021 07:25

mariaschuld added 4 commits November 17, 2021 09:58

move changelog entry

1c1cb9b

remove to obsolete tests

6b350c6

fix confilcts

838c753

forgot to remove one test

86191b7

Base automatically changed from set_num_params_dynamically to master November 17, 2021 09:08

mariaschuld added 6 commits November 17, 2021 11:09

backup

a68ced6

undo scatch change

3209239

Merge branch 'set_num_params_dynamically' into remove-par-domain

c3b695f

Merge branch 'master' into remove-par-domain

695c357

add test to increase cov

c003d5e

Merge branch 'master' into remove-par-domain

028b132

mariaschuld commented Nov 17, 2021

View reviewed changes

black

eaef6e6

mariaschuld requested a review from anthayes92 November 17, 2021 11:52

anthayes92 approved these changes Nov 17, 2021

View reviewed changes

mariaschuld added 2 commits November 17, 2021 19:22

fix conflict

4a79627

Merge branch 'master' into remove-par-domain

5d56bc0

mariaschuld merged commit 4078456 into master Nov 18, 2021

mariaschuld deleted the remove-par-domain branch November 18, 2021 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OpRefactor] Remove par_domain attribute #1907

[OpRefactor] Remove par_domain attribute #1907

mariaschuld commented Nov 17, 2021

codecov bot commented Nov 17, 2021 •

edited

Loading

mariaschuld Nov 17, 2021

mariaschuld Nov 17, 2021

mariaschuld Nov 17, 2021

mariaschuld Nov 17, 2021

mariaschuld Nov 17, 2021

mariaschuld Nov 17, 2021

mariaschuld Nov 17, 2021

anthayes92 Nov 17, 2021

mariaschuld commented Nov 17, 2021

anthayes92 left a comment

anthayes92 Nov 17, 2021

anthayes92 Nov 17, 2021

anthayes92 Nov 17, 2021

anthayes92 Nov 17, 2021

anthayes92 Nov 17, 2021

anthayes92 Nov 17, 2021

mariaschuld commented Nov 17, 2021


		class TestObservableInstantiation:
		"""Test that wires are specified when a qml.operation.Observable is instantiated"""

		@@ -2283,7 +2276,6 @@ class TestSGate(qml.operation.Operation):
		matrix = np.array([[0, 1], [1, 0]])
		num_params = 0

[OpRefactor] Remove par_domain attribute #1907

[OpRefactor] Remove par_domain attribute #1907

Conversation

mariaschuld commented Nov 17, 2021

codecov bot commented Nov 17, 2021 • edited Loading

Codecov Report

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

mariaschuld commented Nov 17, 2021

anthayes92 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

mariaschuld commented Nov 17, 2021

codecov bot commented Nov 17, 2021 •

edited

Loading